Metadata for JRA55
#286
Metadata for JRA55
#286
Conversation
|
sounds good |
|
Another change I made is that |
|
Tests should pass so this is ready to merge / review |
simone-silvestri
requested review from
navidcy and
glwagner
and removed request for
navidcy
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #286 +/- ##
=====================================
Coverage 0.00% 0.00%
=====================================
Files 35 38 +3
Lines 2284 2230 -54
=====================================
+ Misses 2284 2230 -54 ☔ View full report in Codecov by Sentry. |
|
This PR should be ready to merge |
…ean.jl into ss/metadata-for-everything
| @@ -156,7 +129,7 @@ function ECCOFieldTimeSeries(metadata::ECCOMetadata, grid::AbstractGrid; | |||
| inpainting isa Int && (inpainting = NearestNeighborInpainting(inpainting)) | |||
| backend = ECCONetCDFBackend(time_indices_in_memory, metadata; on_native_grid, inpainting, cache_inpainted_data) | |||
|
|
|||
| times = ECCO_times(metadata) | |||
| times = native_times(metadata) | |||
There was a problem hiding this comment.
this used to be called dates, is it times now?
|
|
||
| struct Metadata{D, V} | ||
| name :: Symbol | ||
| dates :: D |
There was a problem hiding this comment.
I see dates here but elsewhere times. Pick one...
There was a problem hiding this comment.
dates it better, times has no reference
| ======= | ||
| An array of time differences in seconds. | ||
| """ | ||
| function native_times(metadata; start_time=first(metadata).dates) |
There was a problem hiding this comment.
| function native_times(metadata; start_time=first(metadata).dates) | |
| function native_times(metadata; start_date=first(metadata).dates) |
its mixing up the meaning of "time" and "date"
There was a problem hiding this comment.
I'm not sure we're ready to make this HUGE leap right now in the backdoor of this PR. We should discuss a bit more because I believe the concept of "time" and "date" is used differently by eg DateTime. There the "date" is a calendar day and time subdivides day. But this function creates a different concept for the two.
We need to have a discussion about the naming here because it is absolutely crucial. Note also that in Oceananigans, we would like to support DateTime for Clock.time natively. This name conflicts with that, obviously.
Maybe native_float_times or something.
There was a problem hiding this comment.
this was just moved from a file to another. We can open an issue to discuss this fact maybe
There was a problem hiding this comment.
the sneaky introduction occurred in a prior PR
| times = zeros(length(metadata)) | ||
| for (t, data) in enumerate(metadata) | ||
| date = data.dates | ||
| time = date - start_time |
There was a problem hiding this comment.
| time = date - start_time | |
| delta = date - start_time |
This PR is an initial proposal to generalize
ECCOMetadatatoMetadataand rework the JRA55 module to useMetadata. In this way, we can have different JRA55 versions (repeat year and multiple year) and we can define adownload_datasetfunction to download the dataset independently of using JRA55 as we can do for ECCOThis PR also removes the ability to generate a
JRA55FieldTimeSeriesdirectly interpolated on the ocean grid, since we need to interpolate anyways when we compute fluxescloses #182