Skip to content

Fix/cfradial2 global attrs assignment - #384

Open
chfer wants to merge 6 commits into
openradar:mainfrom
chfer:fix/cfradial2-global-attrs-assignment
Open

Fix/cfradial2 global attrs assignment#384
chfer wants to merge 6 commits into
openradar:mainfrom
chfer:fix/cfradial2-global-attrs-assignment

Conversation

@chfer

@chfer chfer commented May 28, 2026

Copy link
Copy Markdown
Contributor

This PR fixes a bug in the to_cfradial2 function (located in xradar/io/export/cfradial2.py, line 79).
At that line, the code used:

root = dtree["/"].to_dataset()

The intention is for root to reference the root Dataset so that the following lines can assign the global attributes of the CfRadial2 file. However, DataTree.to_dataset() returns a copy of the dataset, not the live dataset stored in the tree. As a result, the global attributes were being written to this temporary copy and never persisted in the actual DataTree.
The first commit in this branch (fix/cfradial2-global-attrs-assignment, 0eaf495) adds a pytest, tests/io/test_cfradial2.py::test_to_cfradial2_global_attrs, which demonstrates that the global attributes produced by to_cfradial2 do not match the expected values for a CfRadial2 file.
The second commit (3585b62) fixes the issue by replacing:

root = dtree["/"].to_dataset()

with:

root = dtree["/"].ds

Using .ds ensures that the function operates on the actual dataset stored in the DataTree, making the global attribute assignment effective.

@codecov

codecov Bot commented May 28, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.25%. Comparing base (7b81ba0) to head (f601cfc).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #384      +/-   ##
==========================================
+ Coverage   94.23%   94.25%   +0.02%     
==========================================
  Files          29       29              
  Lines        6452     6478      +26     
==========================================
+ Hits         6080     6106      +26     
  Misses        372      372              
Flag Coverage Δ
unittests 94.25% <100.00%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@kmuehlbauer

Copy link
Copy Markdown
Collaborator

@chfer Thanks for starting this. This may need a bit of discussion here. This export function was not meant to add these attributes to the original dataset, but only to add or overwrite these attributes to be written to disk.

So if you do something like this with your approach:

dtree.to_cfradial2(fname2)
dtree.to_cfradial1(fname1)

would have created for test: xradar v0.12 CfRadial2 export: xradar v0.12 CfRadial1 export in the CfRadial1 file, even if it was never loaded from CfRadial2 file.

@syedhamidali

Copy link
Copy Markdown
Member

I believe @kmuehlbauer is suggesting something along these lines:

dtree.xradar.to_cf2(fname2)
dtree.xradar.to_cf1(fname1) 

@chfer

chfer commented Jun 9, 2026

Copy link
Copy Markdown
Contributor Author

Thank you for bringing this to my attention. Indeed my proposed solution is too simple and causes undesirable side-effects. After conversion to cfradial2 the original DataTree should be preserved. The first thing that comes to my mind is to create a (deep) copy of the DataTree upon entering the function to_cfradial2 , so that the original DataTree will always be preserved:

def to_cfradial2(dtree, filename, engine=None, timestep=None):
    """
    docstring ...
    """
    # let the local dtree reference a copy and not the original DataTree
    dtree = dtree.copy(deep=True)
    # now safely modify dtree
    ...
    # write DataTree
    dtree.to_netcdf(filename, engine=engine)

Maybe a shallow copy of dtree is sufficient, I still have to find this out.

@chfer
chfer marked this pull request as draft June 9, 2026 15:50
@chfer

chfer commented Jun 16, 2026

Copy link
Copy Markdown
Contributor Author

In this last commit, the to_cfradial2 function was updated to create a copy of the input DataTree and use that copy for CfRadial2 export. This ensures that the original DataTree remains unchanged after the function returns.

In principle, a shallow copy would be sufficient: to_cfradial2 creates a new DataTree objects for each sweep, and the global attributes are created or replaced rather than mutated in place. However, to make the code more future-proof, I ultimately chose a deep copy, assuming that the additional memory pressure from radar files with multiple elevations and moments will remain manageable.

@kmuehlbauer

Copy link
Copy Markdown
Collaborator

@chfer Is this still draft? We usually wait for the PR author to set this as ready for review.

@chfer
chfer marked this pull request as ready for review July 14, 2026 14:05
@chfer
chfer marked this pull request as draft July 28, 2026 08:10
@chfer
chfer force-pushed the fix/cfradial2-global-attrs-assignment branch from dae89d4 to c75a777 Compare August 12, 2026 14:04
Christophe Férauge added 4 commits August 13, 2026 11:16
 *  A test “tests/io/test_cfradial2.py::test_to_cfradial2_global_attrs” was added to check if the global attributes of a CfRadial2 file created by the to_cfradial2 function are present and have the expected values.

 * Former test needs a minimal DataTree, which is also needed by test_to_cfradial2_selects_default_engine. To avoid repetition a pytest fixture minimal_dtree wass added in “tests/io/test_cfradial2.py.

* The test “tests/io/test_cfradial2.py::test_to_cfradial2_global_attrs” demonstrates that the to_cfradial2 function fails to set global attributes correctly.
 * In the function to_cfradial2 (file xradar/io/export/cfradial2.py, line 79), the original statement 'root = dtree[/].to_dataset()' was replaced with 'root = dtree[/].ds'.This change matters because DataTree.to_dataset() returns a copy of the dataset, while DataTree.ds gives a direct reference to the dataset stored in the tree. Using .ds ensures that modifications affect the actual DataTree content instead of a detached copy.

* The test “tests/io/test_cfradial2.py::test_to_cfradial2_global_attrs” demonstrates now that the to_cfradial2 function succeeds in setting the global attributes as expected.

* Add a history.md entry for global attribute assignement fix in to_cfradial2.
 * ADD: In to_cfradial2, create a copy of the input DataTree and modify only that copy for CfRadial2 export. The original DataTree remains unchanged and can still be used after the function returns.

 * TST: Add test_to_cfradial2_preserves_input_dtree to verify that to_cfradial2 does not mutate the input DataTree.

 * DOC: Add a history entry documenting that to_cfradial2 now preserves the input DataTree.
…on.md

 * set first_dim=time when creating dtree3 for CfRadial2-consistent dimension ordering

 * set optional=False so dtree3 matches variables written by to_cfradial2

 * make the roundtrip comparison validate real equivalence instead of relying on prior in-place mutation behavior

 * updated history.md
@chfer
chfer force-pushed the fix/cfradial2-global-attrs-assignment branch from c75a777 to 7614f25 Compare August 13, 2026 10:06
@chfer

chfer commented Aug 13, 2026

Copy link
Copy Markdown
Contributor Author

The xradar notebook tests - linux (3.13, 2) failure is already present on main (CI #1360, commit 7b81ba0) and is caused by zarr.open_group() rejecting the zarr_format argument. Therefore this appears unrelated to PR #384.

image

@chfer

chfer commented Aug 13, 2026

Copy link
Copy Markdown
Contributor Author

The notebook rendered from CfRadial1_Model_Transformation.md started failing after commit 32d8c37.

The failure occurs in the section “Roundtrip with xradar.io.to_cfradial2”, where the workflow is:

  1. Build dtree3 from a CfRadial1 file with xd.io.open_cfradial1_datatree.
  2. Write test_cfradial2.nc from dtree3 using xd.io.to_cfradial2.
  3. Read test_cfradial2.nc into dtree4 using xarray.open_datatree.
  4. Compare dtree3 and dtree4 in a roundtrip assertion.

On main, this test passed, but it was not validating the intended behavior: xd.io.to_cfradial2 mutated dtree3 in place, so dtree3 and dtree4 were effectively guaranteed to match.

After commit 32d8c37, xd.io.to_cfradial2 uses a deep copy instead of mutating the input. This exposed real differences between dtree3 and dtree4, causing the assertion to fail for two reasons:

  1. Dimension mismatch:
    dtree3 is built with first_dim=auto by default, so moment data in volume scans may use azimuth as first dimension.
    dtree4, as explicit CfRadial2 output, uses time as first dimension.
    Fix: build dtree3 with first_dim="time".

  2. Optional-variable mismatch:
    dtree3 includes optional variables by default (optional=True in xd.io.open_cfradial1_datatree).
    During conversion, optional variables are not preserved in the CfRadial2 output, so dtree4 has fewer variables.
    Fix: build dtree3 with optional=False.

With both conditions applied, the roundtrip assertion succeeds:

dtree3 = xd.io.open_cfradial1_datatree(filename, optional_groups=True, first_dim="time", optional=False)

@syedhamidali
syedhamidali marked this pull request as ready for review August 23, 2026 05:29
…ng to CfRadial2

 * In model.py, the dictionaries required_global_attrs and optional_root_attrs were adapted to allow type checking of inserted global attributes and, where applicable, to check whether the given values are allowed.

 * These dictionaries were also made immutable by using MappingProxyType, with allowed values enumerated as a tuple instead of a list.

 * In cfradial2.py, the function to_cfradial2 was adapted so it now accepts a global_attrs parameter, through which global attributes to add or override can be specified.

 * A pytest function, test_to_cfradial2_global_attrs_override, was added to test overriding/manually inserting global attributes in to_cfradial2.

 * Before being applied, the contents of the global_attrs parameter are validated by a dedicated function, validate_global_attrs, in model.py. A warning is issued for unknown attributes; exceptions are raised when the type or value of a given global attribute is invalid.

 * A pytest function, test_validate_global_attrs_valid_and_invalid_cases, was added for the validate_global_attrs function.

 * Updated history.md.
@chfer

chfer commented Aug 25, 2026

Copy link
Copy Markdown
Contributor Author

Add support for manually setting global attributes when exporting to CfRadial2

Summary

When exporting to CfRadial2/FM301 not all global attributes can be derived from the imported DataTree. Sometimes some global attributes have to be overridden. This commit adds a global_attrs parameter to to_cfradial2(), allowing callers to add or override root-group global attributes at export time (e.g. Conventions, title), with validation against the CfRadial2/FM301 attribute schema.

Changes

model.py

  • Converted required_global_attrs and optional_root_attrs to MappingProxyType-based immutable schemas (GlobalAttrSchema), with allowed values enumerated as tuples instead of lists, to enable type/value checking of supplied global attributes.
  • Added validate_global_attrs(), which validates a global_attrs dict against the schema:
    • Raises TypeError if a value's type doesn't match the expected type.
    • Raises ValueError if a value isn't one of the allowed values for that attribute.
    • Emits a UserWarning for attribute names not recognized as CfRadial2/FM301 global attributes.

cfradial2.py

  • to_cfradial2() now accepts an optional global_attrs: dict[str, object] | None parameter. Supplied attributes are validated via validate_global_attrs() and applied to the root group after xradar's own attribute handling (Conventions, version, history), so user-supplied values take precedence.

Tests

  • tests/test_model.py::test_validate_global_attrs_valid_and_invalid_cases — covers valid attrs, unknown-attribute warning, type-mismatch error, and value-mismatch error.
  • tests/io/test_cfradial2.py::test_to_cfradial2_global_attrs_override — verifies global_attrs both overrides an internally-set attribute (Conventions) and adds a new one (title), while leaving version/history handling intact.

No breaking changes: global_attrs defaults to None, preserving existing behavior when unused.

 * Remove the unsupported ReadOnly attribute from the global attribute schema.

 * Update history.md.
@chfer

chfer commented Aug 26, 2026

Copy link
Copy Markdown
Contributor Author

In commit 4c0c109 several tests involving model.py failed under Python 3.11 because the ReadOnly type qualifier is only available from Python 3.13 onward. This is now corrected in commit f601cfc : the unsupported ReadOnly qualifier was removed from GlobalAttrSpec.
This does not affect the intended behavior: schema values use immutable types (str, type, and tuple), and the global attribute schemas are exposed through MappingProxyType , which prevents modification of the
outer mappings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants