-
Notifications
You must be signed in to change notification settings - Fork 298
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
Fix Rayleigh correction to use the same datatype as the input data #2954
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2954 +/- ##
=======================================
Coverage 96.09% 96.09%
=======================================
Files 377 377
Lines 55110 55125 +15
=======================================
+ Hits 52960 52975 +15
Misses 2150 2150
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Pull Request Test Coverage Report for Build 11517663375Details
💛 - Coveralls |
@pnuu so is the plan for this to be closed and not used? Theoretically pyspectral should have all the changes for dtype stuff, right? Also I really thought I got pyspectral working and retaining dtypes. Shoot. |
Well, maybe the tests I've added should stay. I'll have a look at how much needs to be done to Pyspectral so that it doesn't upcast stuff, and whether there are still something that is needed on Satpy. Like a kwarg or something. |
I'm converting this to a draft while I inspect the Pyspectral side of things. |
Right. There was nothing wrong in Pyspectral, it was just the clipping of input value of |
So in short: this PR makes sure the inputs to Rayleigh correction has proper data type to keep the data from being upcast. And adds the unrelated checks to other tests. |
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.
Looks good, please just add some assert for the computed dtype in some places also
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
@djhoese do you want to look at this? otherwise it's good to merge |
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
This is workaround for the Pyspectral Rayleigh correction that always uses
float64
datatype.This includes also few additional dtype checks for other modifiers and enhancements. Those changes are here because I originally started this to hunt a data upcasting in true color composite, but didn't end up finding the final issue and didn't want to discard all the changes.
I'll create an issue to Pyspectral to fix the data upcasting. Hopefully tomorrow. Or a direct fix if I see a simple solution.