Cf/Radial1 writer

[syedhamidali]

• Closes Cf/Radial1 writer #124 (Preliminary)
• Tests added
• Changes are documented in history.md

I have incorporated the CFRadial1 writer into the code and tested it with four different CFRadial1 data files. It successfully processed three of the files. While it currently works with full radar volumes, I acknowledge that further refinement and compatibility with sweeps are necessary. I welcome any suggestions for improvement and am open to any modifications that enhance its suitability.

I also ran pre-commit run --all-files.

Thanks
Hamid

[kmuehlbauer]

@syedhamidali I've fixed an issue on main to get the CI started here. I'll have a closer look later today if no one beats me to it.

[codecov]

Codecov Report

Merging #126 (e807122) into main (e10a697) will increase coverage by 0.10%.
The diff coverage is 91.83%.

@@            Coverage Diff             @@
##             main     #126      +/-   ##
==========================================
+ Coverage   88.21%   88.31%   +0.10%     
==========================================
  Files          19       20       +1     
  Lines        3334     3431      +97     
==========================================
+ Hits         2941     3030      +89     
- Misses        393      401       +8     

Flag	Coverage Δ
unittests	88.31% <91.83%> (+0.10%)	⬆️

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

Files	Coverage Δ
xradar/io/export/__init__.py	100.00% <100.00%> (ø)
xradar/model.py	96.19% <100.00%> (ø)
xradar/io/export/cfradial1.py	91.66% <91.66%> (ø)

[syedhamidali]

So, @kmuehlbauer, could you please explain how Codecov works? My understanding is that we need to create some tests for it. However, I couldn't find any tests for to_cfradial2, and I'm hesitant to make changes without a clear understanding of how it functions. I'd appreciate your assistance in this matter. Thank you!

[kmuehlbauer]

@syedhamidali Currently we use the notebooks to test this. That's no full featured unittest as it just checks that it runs without raising an error.

We'd decided to add tests while touching the codebase.

[syedhamidali]

Thank you for this info! So, what is the solution here?

[kmuehlbauer]

OK, I can't give a very detailed answer now, but the idea would be to create some roundtrip tests first. Where we write and read back some predefined data and compare it against each other.

Finally, we would like to have tests for each function.

[kmuehlbauer]

@syedhamidali Thanks for tackling this issue. I've added a couple comments and suggestions.

For adding tests, maybe a simple roundtrip will be sufficient?

1. read CfRadial1 file
2. export to_cfradial1
3. read back and compare with 1

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[kmuehlbauer]

@syedhamidali Let's try to get this in by tomorrow or Thursday latest. I'm really pushing for a release 0.4.0 on Thursday.

[syedhamidali]

@kmuehlbauer Notebook for the tests or for the example?

[kmuehlbauer]

Good question, do you think it would make sense to combine it?

[syedhamidali]

Separating them would be a better approach as it simplifies the user experience by not exposing the tests directly. Additionally, I attempted a this approach, and when we save it to cfradial1, the xarray does not preserve its mask. This discrepancy in the masking could be a potential reason for the lack of identity between the two datasets. What are your thoughts on this?

[syedhamidali]

But this way it seems they are identical

[kmuehlbauer]

OK, then I'd suggest an example notebook and unit tests under xradar/tests/io -> test_cfradial1.py.

You could use xr.testing.assert_identical(ds1, ds2) for testing.

[syedhamidali]

OK, then I'd suggest an example notebook and unit tests under xradar/tests/io -> test_cfradial1.py.

You could use xr.testing.assert_identical(ds1, ds2) for testing.

I tried that one, check this out #126 (comment), I feel that is masked_arrays/fill_value vs NaNs

[kmuehlbauer]

xr.testing.assert_identical(dtree["sweep_0"].ds, dtree2["sweep_0"].ds)
xr.testing.assert_equal(dtree["sweep_1"].ds, dtree2["sweep_1"].ds)
xr.testing.assert_equal(dtree["sweep_2"].ds, dtree2["sweep_2"].ds)

Something like this works for me.

[kmuehlbauer]

I attempted a this approach, and when we save it to cfradial1, the xarray does not preserve its mask. This discrepancy in the masking could be a potential reason for the lack of identity between the two datasets. What are your thoughts on this?

I think you can't write masked arrays to netcdf with xarray. You would need _FillValue attribute in any case. Maybe I'm misunderstanding the issue.

[kmuehlbauer]

@syedhamidali I think the roundtrip test is sufficient for now. Please let me know when this is ready for merge. There might be some clean-up needed in the CfRadial1 Notebook. Maybe we can have the release already tomorrow. 🚀

[syedhamidali]

I tried to make a notebook, but the problem is that I don't know yet how this datatree.DataTree works and its documentation is also not clear enough. Would you like to take a look at this notebook and suggest your changes, so that I can just upload that to the notebooks? Here https://github.com/syedhamidali/test_scripts/blob/test/CfRadial1_Export.ipynb

[kmuehlbauer]

@syedhamidali The notebook looks nice for the first setup, you can extend it any time by adding additional information to it.

Add it to the examples and we are almost ready to get this in.

[kmuehlbauer]

@syedhamidali This is a great first contribution! I've added another couple of comments and suggestions. After addressing those we can get this in and have a release party.

[kmuehlbauer]

Please also add an entry to docs/history.md!

[syedhamidali]

Hello Kai,

Just a quick question before I run pre-commit, should I use tempfile generated filename, or just comment this xd.io.to_cfradial1 statement in the notebook

[kmuehlbauer]

Just a quick question before I run pre-commit, should I use tempfile generated filename, or just comment this xd.io.to_cfradial1 statement in the notebook

Which notebook? If you mean your example notebook just keep the filename and write to the current directory.

[kmuehlbauer]

OK, we are very close now @syedhamidali. There are minimal tweakings needed to hook the example notebooks at the proper place in the docs. I can do this work after merging this while preparing the release. WDYT? Shall we merge?

[syedhamidali]

OK, we are very close now @syedhamidali. There are minimal tweakings needed to hook the example notebooks at the proper place in the docs. I can do this work after merging this while preparing the release. WDYT? Shall we merge?

Thank you, @kmuehlbauer, please go ahead whenever you're ready.

[kmuehlbauer]

Thanks @syedhamidali, 👍 💯 !

[mgrover1]

Thanks for all your hard work here @syedhamidali !!