Repository navigation
Conversation
* Check time_coverage_start and time_coverage_end across supported backends. * Assert scalar string values and valid FM301 units, calendar, and standard_name attributes. * Mark the tests as strict xfail until the root DataTree attributes are implemented.
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## main #519 +/- ##
===========================================
- Coverage 96.04% 82.81% -13.24%
===========================================
Files 29 30 +1
Lines 6950 7494 +544
===========================================
- Hits 6675 6206 -469
- Misses 275 1288 +1013
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| FM301 requires both root variables to be strings carrying the string | ||
| attributes ``units`` ("seconds since YYYY-MM-DDThh:mm:ssZ"), ``calendar`` | ||
| (a CF calendar name) and ``standard_name`` (``"time"``). |
There was a problem hiding this comment.
Nice, why can't those standards not agree with each other?
There are only two options, string without units or numeric dtype with units.
If it has units described in cf-conventions than it is a CF time coordinate and need a numeric dtype.
There was a problem hiding this comment.
I wonder if this is not an error in the FM301-2022 spec ? I always understood that FM301 is a subset from CfRadial2, so I would not expect such a difference in type for the same root variable. Also using a string to express a pure numeric value seems weird. The official FM301 compliance checker ( https://github.com/wmo-ipet-owr/FM301_CC ) also expects a string for time_coverage_start and time_coverage_end (see file https://github.com/wmo-ipet-owr/FM301_CC/blob/main/cf_radial_metadata_Final.json at line 141 and 164) . On the other hand the WMO FM301 C++ API ( https://gitlab.com/bom-radar/fm301 ) outputs time_coverage_start and time_coverage_end root variables as double when you convert an ODIM file to FM301.
I will inform the persons I know from WMO who deal with FM301.
There was a problem hiding this comment.
An error in the spec, that's my impression, too. Thanks for taking care!
* Assert that time_coverage_start and time_coverage_end have a float64 dtype across supported backends, as required despite the FM301 specification describing them as strings.
|
Same here @chfer, after working through the PR backlog, this can be picked up again. Thanks! |
Description
This draft PR aims to correct the values and metadata of the root variables time_coverage_start and time_coverage_end.
The initial commit adds only a test demonstrating that these variables currently:
⠀No implementation changes are included yet.
Specification clarification needed
Completing the fix requires resolving a difference between the data types prescribed by the two specifications:
This PR remains a draft pending agreement on the target specification and the intended representation of these values. The test assertions may need to be adjusted accordingly before implementing the fix..
Development note
The test was written with assistance from GitHub Copilot. I reviewed all generated code and adapted it where necessary.
Fixes: #520