-
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 dtype promotion in SunZenithReduction
#2950
Fix dtype promotion in SunZenithReduction
#2950
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2950 +/- ##
=======================================
Coverage 96.08% 96.08%
=======================================
Files 377 377
Lines 55125 55144 +19
=======================================
+ Hits 52967 52986 +19
Misses 2158 2158
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 11669217385Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
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.
So this fixes some extra memory being used inside a dask task, but doesn't change the final dtype of the output of this "angles" step (which was correct), right?
Side note: Based on this math website, can't this equation be rewritten as log-base2 of reduction_factor + 1
?
https://www.cuemath.com/algebra/log-rules/
change of base rule: loga b = (logc b) / (logc a)
With the current |
And well spotted, the |
Hopefully that is still mathematically equivalent and I'm not misreading it. |
It is, the tests pass (at least locally). |
I restarted CI, MacOS tests failed on setup. |
The
SunZenithReduction
modifier used for example in the built-in FCItrue_color
composite has a division withnp.log(2)
that changes thefloat32
input data tofloat64
. This PR is a simple fix for that.The upcasting is hidden behind some other part of the code because even without this fix the completed product was
float32
.This small change also reduced the memory usage for my 12-composite set by roughly 800 MB.
With
main
:With this PR: