-
Notifications
You must be signed in to change notification settings - Fork 94
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.
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
Add support for fill_value
and set_empty_bucket_to
in BucketResampler get_sum
#602
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #602 +/- ##
==========================================
- Coverage 94.01% 93.99% -0.03%
==========================================
Files 92 86 -6
Lines 13836 13782 -54
==========================================
- Hits 13008 12954 -54
Misses 828 828
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
I addressed all comments, and most importantly I switched all bucket tests to pytest. I realised there are many ways to do this (like do we need a base test class containing all tests?), so please let me know if you like it like this. I also used fixtures with module scope to optimise it a bit. I also parametrised the get_sum arguments tests into one test as proposed. It took a while to figure out the combination and simplification of the test flow, but indeed I think it improved the test readability, and made the testing of more corner cases much easier. So thanks for pushing me to do it :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small suggestions, but otherwise looks good. I'd really like @pnuu to review this given he's the original author (right?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just the one nitpick-y change that Dave pointed out.
Hi @djhoese , thanks for the review and the ping, indeed it would be great to have this in the next release. I have pushed the modifications as requested. I believe it should close the PR, yes, as it implements the prosposed functionalities, even with a better granularity using the |
I agree this can close my old PR because it considers both nan and fill_value. Thanks! |
This PR adds support for
fill_value
andset_empty_bucket_to
in BucketResamplerget_sum
. This is a nice feature, but most importantly it fixes the satpybucket_sum
interface.Closes pytroll/satpy#2805, closes pytroll/satpy#1746, closes #354
git diff origin/main **/*py | flake8 --diff