Read ECCO-Darwin field from NASA Earthdata

[seamanticscience]

Adding functionality and interface to initialize and force BGC ClimaOcean simulations using ECCO-Darwin fields

[simone-silvestri]

Nice. I see there is a lot of code duplication, I think we can avoid it by, instead of writing a new metadata type, writing a new version type that contains the additional information we need to wrangle the data.

I would propose therefore to have

struct ECCO4DarwinMonthly{T, D}
    time_step_size :: T
    reference_date :: D
end

so that it is possible to define an alias

const ECCO4DarwinMetadata = ECCOMetadata{<:Any, <:ECCO4DarwinMonthly}

I think it would be nice to have only one Metadata type for all the data we wrangle with specialized version fields for all the different data (I was going that route in #286). It a bit ambitious, but it is still possible with this change

[simone-silvestri]

Here you can define a function read_ECCO_data(metadata, path) that dispatches on the version of the metadata and returns

    shortname = short_name(metadata)

    if variable_is_three_dimensional(metadata)
        data = ds[shortname][:, :, :, 1]
        data = reverse(data, dims=3)
    else
        data = ds[shortname][:, :, 1]
    end        

by default and

model_metadata = ECCODarwinModelMeta(replace(path, "data"=>"meta"))
mesh_grid = ECCODarwinMeshGrid(metadata.version)
data = ECCODarwinModelData(path, model_metadata, mesh_grid)

for ECCODarwin* versions. In this way you can reuse the current ECCO_field implementation

[seamanticscience]

Yes, that sounds like a plan.

[simone-silvestri]

Suggested change

	Base.length(metadata::ECCODarwinMetadata) = length(metadata.dates)
	Base.size(data::ECCODarwinMetadata{<:Any, <:Any, <:ECCO270DarwinMonthly}) = (1080, 540, 50, length(data.dates))
	Base.size(data::ECCODarwinMetadata{<:Any, <:Any, <:ECCO4DarwinMonthly}) = (360, 180, 50, length(data.dates))
	Base.length(metadata::ECCOMetadata) = length(metadata.dates)
	Base.size(data::ECCOMetadata{<:Any, <:Any, <:ECCO270DarwinMonthly}) = (1080, 540, 50, length(data.dates))
	Base.size(data::ECCOMetadata{<:Any, <:Any, <:ECCO4DarwinMonthly}) = (360, 180, 50, length(data.dates))

[simone-silvestri]

does this function actually read the data or provide a path to the data?

[seamanticscience]

It reads a text ("meta") file that contains information about the "data" file - in particular the binary precision of the MITgcm output.

[seamanticscience]

Thanks for the comments @simone-silvestri !

[glwagner]

Just a note, "ECCO4" refers to ECCO version 4, but certainly "ECCO 270" does not refer to ECCO version 270. Do we have ideas for clearer naming?

[seamanticscience]

It's unclear to me, at least, if LLC270 will be ECCOv5, or if it's a stop-gap for a high res run, that's not CS510 (which is what you're calling ECCO2). Also, FYI there are 5 "releases" of ECCO version 4. ECCODarwin has results on V4r4 and V4r5, but I left that out for now, and just stuck to "ECCO4".

[glwagner]

Okay, well the key for the metadata is to define the grid and a way to set the variables. So if the different releases occupy the same pattern, then loading data from different releases can be described through properties of the types rather than the type names; for example ECCO4Monthly(release=5) or whatever.

We can call ECCO2 CS510 btw. As I understand it, "ECCO v2" refers to the run, whereas CS510 is the name of the grid "cubed sphere 510 points" or whatever. So the name "CS510" is less specific than "ECCO2"; there are many runs with CS510 that are not ECCO, or not on Earth at all. Similarly LLC270 is the name of the grid, right?

[glwagner]

Please try to proofread the code carefully. There are some formatting inconsistencies, like spacing missing after commas. Reading the code carefully is useful because you may also catch typos.

The style is a bit inconsistent with the rest of the code. If a function is not a "constructor" (returning a type of the same name as the function), then it should use snake_case rather than TitleCase. Nowhere should we use snakeCase. Finally, for continuing functions and parentheses we generally line text up rather than indent, eg

some_function(a,
              b,
              c)

rather than

some_function(a,
    b,
    c
)

The main point is that the code will have to be proofread carefully anyways, and it's a good idea to write with good style and punctuation, same as writing a paper.

[seamanticscience]

Test failing on something external:

[ Info: Testing that we can download ECCO data...
Availability of ECCO data: Error During Test at /home/runner/work/ClimaOcean.jl/ClimaOcean.jl/test/test_downloading.jl:12
  Got exception outside of a @test
  RequestError: HTTP/1.1 401 Unauthorized while requesting https://ecco.jpl.nasa.gov/drive/files/Version4/Release4/interp_monthly//SIarea/1993/SIarea_1993_01.nc

[simone-silvestri]

I am not sure that you have an ECCO_USERNAME and ECCO_PASSWORD set up as environmental variables in your fork of the repository, which would lead to an error like this. Can you try to add them as secrets and see if the tests pass?

[seamanticscience]

Ah, I see, done! I had no idea that you could do that!

[seamanticscience]

ECCO-Darwin DIC concentration at the surface, 1993-01-01 00:00:00

[glwagner]

Will be slightly cleaner to use abstract type or type union to express this. This pattern does not generalize well; this makes it more work to change how we express version for example.

[seamanticscience]

as in if metadata.version isa Union{ECCO4Monthly, ECCO4DarwinMonthly}?

[seamanticscience]

Quick note: spotted a few issues that'll need fixing (downloading hFacC by MeshArrays doesn't automatically trigger like I had thought; ECCOFieldTimeSeries for ECCO4DarwinMonthly has some weird behaviour that I will document.)

[seamanticscience]

Also, I added my ECCO details as secrets, but the test still fails, so not sure if I did it right?