Fix/cfradial2 global attrs assignment#384
Conversation
* 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.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #384 +/- ##
=======================================
Coverage 94.20% 94.20%
=======================================
Files 29 29
Lines 6417 6418 +1
=======================================
+ Hits 6045 6046 +1
Misses 372 372
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:
|
|
@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 |
|
I believe @kmuehlbauer is suggesting something along these lines: dtree.xradar.to_cf2(fname2)
dtree.xradar.to_cf1(fname1) |
|
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. |
* 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.
|
In this last commit, the In principle, a shallow copy would be sufficient: |
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:
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:
with:
Using .ds ensures that the function operates on the actual dataset stored in the DataTree, making the global attribute assignment effective.