Skip to content
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.

Sign up for GitHub

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

Jump to bottom

FIX: _FillValue + History in the cfradial1 exporter #132

Merged
merged 9 commits into from
Oct 5, 2023
Merged

FIX: _FillValue + History in the cfradial1 exporter #132

merged 9 commits into from
Oct 5, 2023

Conversation

syedhamidali
Copy link
Contributor

  • Changes are documented in history.md

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

@codecov
Copy link

codecov bot commented Sep 29, 2023 •
edited
Loading

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%) ⬆️

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%) ⬆️
xradar/model.py 96.19% <ø> (ø)

@syedhamidali syedhamidali self-assigned this Sep 29, 2023
@syedhamidali syedhamidali added the enhancement New feature or request label Sep 29, 2023
kmuehlbauer
kmuehlbauer requested changes Oct 2, 2023
View reviewed changes
Copy link
Collaborator

@kmuehlbauer kmuehlbauer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@syedhamidali Thanks for continuing to shape the CfRadial1 exporter. I've added some remarks and questions.

docs/history.md Outdated Show resolved Hide resolved
xradar/io/export/cfradial1.py Outdated Show resolved Hide resolved
xradar/io/export/cfradial1.py Outdated Show resolved Hide resolved
kmuehlbauer
kmuehlbauer approved these changes Oct 2, 2023
View reviewed changes
Copy link
Collaborator

@kmuehlbauer kmuehlbauer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@syedhamidali This is OK with me. I've one issue with the _FillValue but will rely on the expertise of @mgrover1 here. Thanks!

xradar/io/export/cfradial1.py Outdated Show resolved Hide resolved
xradar/io/export/cfradial1.py Outdated Show resolved Hide resolved
@mgrover1
Copy link
Collaborator

mgrover1 commented Oct 2, 2023

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

@kmuehlbauer
Copy link
Collaborator

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
Copy link
Collaborator

@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
Copy link
Contributor Author

@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
Copy link
Contributor Author

@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:

  • Minor error fixed in the Notebook
  • Added xradar version and cfradial1 version to the dataset attributes

mgrover1
mgrover1 approved these changes Oct 4, 2023
View reviewed changes
Copy link
Collaborator

@mgrover1 mgrover1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great to me!

@kmuehlbauer kmuehlbauer enabled auto-merge (squash) October 4, 2023 20:03
auto-merge was automatically disabled October 5, 2023 01:22

Head branch was pushed to by a user without write access

@syedhamidali syedhamidali mentioned this pull request Oct 5, 2023
Closed
@kmuehlbauer kmuehlbauer enabled auto-merge (squash) October 5, 2023 04:03
kmuehlbauer
kmuehlbauer approved these changes Oct 5, 2023
View reviewed changes
Copy link
Collaborator

@kmuehlbauer kmuehlbauer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks @syedhamidali !

@kmuehlbauer kmuehlbauer merged commit 6880b0c into openradar:main Oct 5, 2023
11 checks passed
@kmuehlbauer kmuehlbauer mentioned this pull request Nov 4, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mgrover1 mgrover1 mgrover1 approved these changes

@kmuehlbauer kmuehlbauer kmuehlbauer approved these changes

Assignees

@syedhamidali syedhamidali

Labels
enhancement New feature or request
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@syedhamidali @mgrover1 @kmuehlbauer