Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Does ECCOMetadata have dates or a date? (Should we use a different design for ECCOMetadata?) #224

Open
glwagner opened this issue Nov 8, 2024 · 4 comments
Labels
documentation Improvements or additions to documentation

Comments

@glwagner
Copy link
Member

glwagner commented Nov 8, 2024

The docstring says date

- `date`: The date of the metadata (default: DateTimeProlepticGregorian(1993, 1, 1)).

but the actual function signature requires dates:

dates = DateTimeProlepticGregorian(1993, 1, 1),

I also want to note that I think this design is confusing. The user interface is confusing, and it also makes the source code confusing.

Another design could be to have ECCOMetadata represent a single instance and then use a vector of ECCOMetadata to represent many of them (if that is even necessary).

Anything to make the design more explicit with less hidden suprises.

@glwagner glwagner added the documentation Improvements or additions to documentation label Nov 8, 2024
@glwagner
Copy link
Member Author

glwagner commented Nov 8, 2024

I guess another possibility is to discontinue "single date" support, so users just have to write [date] if they want one date.

@NoraLoose
Copy link
Collaborator

I can confirm that following the documentation example and using

date  = DateTimeProlepticGregorian(1993, 1, 1)
set!(ocean.model, T = ClimaOcean.ECCOMetadata(:temperature; date),
                  S = ClimaOcean.ECCOMetadata(:salinity; date))

throws an error:

MethodError: no method matching ECCOMetadata(::Symbol; date::DateTimeProlepticGregorian)

Closest candidates are:
  ECCOMetadata(::Symbol; dates, version, dir) got unsupported keyword argument "date"
   @ ClimaOcean /pscratch/sd/n/nloose/depot/packages/ClimaOcean/kTfXn/src/DataWrangling/ECCO/ECCO_metadata.jl:70
  ECCOMetadata(::Symbol, ::Any; ...)
   @ ClimaOcean /pscratch/sd/n/nloose/depot/packages/ClimaOcean/kTfXn/src/DataWrangling/ECCO/ECCO_metadata.jl:78
  ECCOMetadata(::Symbol, ::Any, ::Any; dir) got unsupported keyword argument "date"
   @ ClimaOcean /pscratch/sd/n/nloose/depot/packages/ClimaOcean/kTfXn/src/DataWrangling/ECCO/ECCO_metadata.jl:78
  ...


Stacktrace:
 [1] kwerr(::@NamedTuple{date::DateTimeProlepticGregorian}, ::Type, ::Symbol)
   @ Base ./error.jl:165
 [2] top-level scope
   @ In[23]:1

But changing the syntax to

dates  = DateTimeProlepticGregorian(1993, 1, 1)
set!(ocean.model, T = ClimaOcean.ECCOMetadata(:temperature; dates),
                  S = ClimaOcean.ECCOMetadata(:salinity; dates))

works, as expected.

@glwagner
Copy link
Member Author

I guess the global simulation example uses

date = DateTimeProlepticGregorian(1993, 1, 1)
set!(ocean.model, T=ECCOMetadata(:temperature; dates=date),
                  S=ECCOMetadata(:salinity; dates=date))

If we are ok with this syntax we can close this issue. I guess the basic idea is that we can make metadata representing a range of dates:

julia> dates  = DateTimeProlepticGregorian(1993, 1, 1):Month(1):DateTimeProlepticGregorian(1994, 1, 1)
DateTimeProlepticGregorian(1993-01-01T00:00:00):Month(1):DateTimeProlepticGregorian(1994-01-01T00:00:00)

julia> metadata = ECCOMetadata(:temperature; dates)
ECCOMetadata:
├── name: temperature
├── dates: DateTimeProlepticGregorian(1993-01-01T00:00:00):Month(1):DateTimeProlepticGregorian(1994-01-01T00:00:00)
├── version: ClimaOcean.DataWrangling.ECCO.ECCO4Monthly()
└── dir: /Users/gregorywagner/.julia/scratchspaces/0376089a-ecfe-4b0e-a64f-9c555d74d754/ECCO

Then we can index into that metadata like its a vector:

julia> metadata[1]
ECCOMetadata:
├── name: temperature
├── dates: DateTimeProlepticGregorian(1993-01-01T00:00:00)
├── version: ClimaOcean.DataWrangling.ECCO.ECCO4Monthly()
└── dir: /Users/gregorywagner/.julia/scratchspaces/0376089a-ecfe-4b0e-a64f-9c555d74d754/ECCO

which gives the same thing as building the metadata with a single date.

It doesn't feel like the most natural syntax but it's also not horrible. So maybe we can close this issue.

@glwagner
Copy link
Member Author

Also worth noting that the docstring was fixed at some point:

- `dates`: The date(s) of the metadata. Note this can either be a single date,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

No branches or pull requests

2 participants