Fix ECCO metadata_url and urls that both adding a trailing /

[navidcy]

Close #316

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 3 lines in your changes missing coverage. Please review.

Project coverage is 0.00%. Comparing base (3b252db) to head (7a0eb90).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
src/DataWrangling/ECCO/ECCO_metadata.jl	0.00%	3 Missing ⚠️

Additional details and impacted files

@@          Coverage Diff          @@
##            main    #318   +/-   ##
=====================================
  Coverage   0.00%   0.00%           
=====================================
  Files         34      34           
  Lines       1983    1983           
=====================================
  Misses      1983    1983           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[navidcy]

So we can literate this example, but I don't know what to do with the sliders. I don't think we can have sliders in the docs...
At the moment, it looks like:
https://clima.github.io/ClimaOceanDocumentation/previews/PR318/literated/ecco_inspect_temperature_salinity/

[glwagner]

If this fixes a bug, we should add a test to ensure the bug doesn't happen again. How did this bug get through?

[navidcy]

I'm also wondering because there are tests that download ECCO data; I'll look into that.

[simone-silvestri]

Repeated / are usually not accounted for when navigating URLs or a (UNIX) file system.
For example, to access ClimaOcean
https://github.com/CliMA/ClimaOcean.jl/
is equivalent to
https://github.com/CliMA/////ClimaOcean.jl/
or
https://github.com/////CliMA/ClimaOcean.jl
The same goes with cd:
cd /to/my/path and cd /to///my/path/// are equivalent.
With this convention, it is possible to compose directories or join paths without considering whether adding a trailing / will break the path and we can always add a trailing /. So this is not technically a bug, I would call it more a syntax cleanup. However, I am not sure this pattern is also valid when navigating a windows filesystem.

[glwagner]

Ok that is what I suspected --- not a bug, but good to clean up regardless. Let's not mark it as a bug though because that may mislead

[navidcy]

Seems like there are errors due to scalar operations at

https://github.com/CliMA/Oceananigans.jl/blob/e02564df8ab90fb8f994fddf465b10d123fdfc3a/src/Operators/spacings_and_areas_and_volumes.jl#L204

See: https://buildkite.com/clima/climaocean-ci/builds/2367#01945504-9e29-4245-8d55-4e7942f35978/187-526

But how come this only came up now? Is this related to the pin in GPUArrays?

[simone-silvestri]

yeah looks like. Should go away if we use [email protected]

[navidcy]

and none of the tests of Oceananigans caught this?

[navidcy]

and none of the tests of Oceananigans caught this?

OK, none of Oceananigans tests caught this because they are all run within a CUDA.allowscalar()!

https://github.com/CliMA/Oceananigans.jl/blob/e02564df8ab90fb8f994fddf465b10d123fdfc3a/test/runtests.jl#L19

We should remove this CUDA.allowscalar() do ... all-the-tests ... end

cc @glwagner

[simone-silvestri]

I am definitely in favor of that. allowscalar should be more granular and used only when necessary.