Repository navigation
FIX: Correct values and mandatory attributes of root variables time_coverage_start and time_coverage_end #519
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
Draft
chfer
wants to merge
3
commits into
openradar:main
Choose a base branch
from
chfer:fix/correct-time_coverage-root-vars
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,110 @@ | ||
| #!/usr/bin/env python | ||
| # Copyright (c) 2026, openradar developers. | ||
| # Distributed under the MIT License. See LICENSE for more info. | ||
|
|
||
| """ | ||
| Tests for the FM301-2022 attributes of the root variables | ||
| ``time_coverage_start`` and ``time_coverage_end``. | ||
|
|
||
| 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"``). | ||
| """ | ||
|
|
||
| import re | ||
| from datetime import datetime | ||
|
|
||
| import pytest | ||
|
|
||
| import xradar as xd | ||
|
|
||
| UNITS_PREFIX = "seconds since " | ||
| TIMESTAMP_PATTERN = re.compile(r"^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}Z$") | ||
| CALENDARS = { | ||
| "gregorian", | ||
| "standard", | ||
| "proleptic_gregorian", | ||
| "noleap", | ||
| "365_day", | ||
| "all_leap", | ||
| "366_day", | ||
| "360_day", | ||
| "julian", | ||
| "none", | ||
| } | ||
|
|
||
| # One backend per way the root group is built: | ||
| # - shared path (_get_required_root_dataset -> _assign_root): odim, gamic, | ||
| # furuno, rainbow, iris | ||
| # - direct _assign_root callers: nexradlevel2, uf | ||
| # - own root implementation: cfradial1, cfradial2 | ||
| ENGINES = [ | ||
| pytest.param(("odim", "odim_file"), id="odim"), | ||
| pytest.param(("gamic", "gamic_file"), id="gamic"), | ||
| pytest.param(("furuno", "furuno_scn_file"), id="furuno"), | ||
| pytest.param(("rainbow", "rainbow_file"), id="rainbow"), | ||
| pytest.param(("iris", "iris0_file"), id="iris"), | ||
| pytest.param(("nexradlevel2", "nexradlevel2_file"), id="nexradlevel2"), | ||
| pytest.param(("uf", "uf_file_1"), id="uf"), | ||
| pytest.param(("cfradial1", "cfradial1_file"), id="cfradial1"), | ||
| pytest.param(("cfradial2", "cfradial2_file"), id="cfradial2"), | ||
| ] | ||
|
|
||
| # module-wide expected-failure marker | ||
| # Known gap: the attributes are lost / never set on the root variables. | ||
| # strict=True turns these into failures as soon as the gap is fixed. | ||
| pytestmark = pytest.mark.xfail( | ||
| reason="FM301 attributes of time_coverage_* missing on root variables", | ||
| strict=True, | ||
| ) | ||
|
|
||
|
|
||
| # Return the root dataset and close its owning DataTree after each test. | ||
| @pytest.fixture(params=ENGINES) | ||
|
kmuehlbauer marked this conversation as resolved.
Resolved
|
||
| def root_ds(request): | ||
| engine, fixture_name = request.param | ||
| filepath = request.getfixturevalue(fixture_name) | ||
| dtree = xd.open_datatree(filepath, engine=engine) | ||
| try: | ||
| yield dtree.to_dataset(inherit=False) | ||
| finally: | ||
| dtree.close() | ||
|
|
||
|
|
||
| # check the FM301 time coverage attributes on an opened root dataset | ||
| @pytest.mark.parametrize("name", ["time_coverage_start", "time_coverage_end"]) | ||
| def test_time_coverage_fm301_attrs(root_ds, name): | ||
| var = root_ds[name] | ||
|
|
||
| # the variable must be scalar (0-dimensional) | ||
| assert var.ndim == 0 | ||
|
|
||
| # the variable itself is a string (str or bytes, depending on backend) | ||
| assert var.dtype.kind in ("U", "S", "O") | ||
| # if the variable is of object type, it must contain a string | ||
| if var.dtype.kind == "O": | ||
| assert isinstance(var.item(), str) | ||
| # the string must represent a valid number, being parseable as a float | ||
| # according to FM301, the time coverage variable represents the number of seconds since a given reference time | ||
| try: | ||
| float(var.item()) | ||
| except (ValueError, TypeError): | ||
| pytest.fail(f"{name} must represent a valid number of seconds") | ||
|
|
||
| # check if the required FM301 attributes are present and correctly typed | ||
| attrs = var.attrs | ||
| for key in ("units", "calendar", "standard_name"): | ||
| assert key in attrs, f"{name} is missing FM301 attribute {key!r}" | ||
| assert isinstance(attrs[key], str), f"{name}:{key} must be a string" | ||
|
|
||
| # check the format of the units attribute, which should start with the UNITS_PREFIX and contain a valid timestamp | ||
| assert attrs["units"].startswith(UNITS_PREFIX), attrs["units"] | ||
| timestamp = attrs["units"][len(UNITS_PREFIX) :] | ||
| assert TIMESTAMP_PATTERN.fullmatch(timestamp), attrs["units"] | ||
| try: | ||
| datetime.strptime(timestamp, "%Y-%m-%dT%H:%M:%SZ") | ||
| except ValueError: | ||
| pytest.fail(f"{name} units contain an invalid timestamp: {timestamp!r}") | ||
| # check the calendar and standard_name attributes | ||
| assert attrs["calendar"] in CALENDARS | ||
| assert attrs["standard_name"] == "time" | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, why can't those standards not agree with each other?
There are only two options,
stringwithoutunitsor numeric dtype withunits.If it has
unitsdescribed in cf-conventions than it is a CF time coordinate and need a numeric dtype.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
stringfor 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 asdoublewhen 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An error in the spec, that's my impression, too. Thanks for taking care!