search

openradar / xradar

A tool to work in weather radar data in xarray
https://docs.openradarscience.org/projects/xradar
MIT License
85 stars 17 forks source link

FIX: _FillValue + History in the cfradial1 exporter #132

Closed syedhamidali closed 9 months ago

syedhamidali commented 9 months ago
review-notebook-app[bot] commented 9 months ago

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

codecov[bot] commented 9 months ago

Codecov Report

Merging #132 (466fcf3) into main (1ef1ebe) will increase coverage by 0.01%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #132      +/-   ##
==========================================
+ Coverage   88.31%   88.32%   +0.01%     
==========================================
  Files          20       20              
  Lines        3431     3436       +5     
==========================================
+ Hits         3030     3035       +5     
  Misses        401      401
Flag Coverage Δ
unittests 88.32% <100.00%> (+0.01%) :arrow_up:

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

Files Coverage Δ
xradar/io/export/cfradial1.py 92.07% <100.00%> (+0.41%) :arrow_up:
xradar/model.py 96.19% <ø> (ø)
mgrover1 commented 9 months ago

There is no "official" fill value - that is left up to the data providers!

kmuehlbauer commented 9 months ago

There is no "official" fill value - that is left up to the data providers!

Then make it parametrizable? And use netcdf default if not specified?

kmuehlbauer commented 9 months ago

@syedhamidali OK, maybe we can skip that _FillValue issue altogether.

If _FillValue is not given, netcdf4 will take the default value anyway. If _FillValue is set, it will take this _FillValue. So maybe we should just document?

syedhamidali commented 9 months ago

@syedhamidali OK, maybe we can skip that _FillValue issue altogether.

If _FillValue is not given, netcdf4 will take the default value anyway. If _FillValue is set, it will take this _FillValue. So maybe we should just document?

Actually, I tried to manually check the same using pyart prior to this PR, and I noticed the fill values don't get filled in. That's one of the reason for this PR.

syedhamidali commented 9 months ago

@kmuehlbauer , I have dropped the __FillValue, as it is taken care of by default netcdf methods maybe. I think we are good to go with this PR. The changes include: