-
Notifications
You must be signed in to change notification settings - Fork 32
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
use sqrtf to avoid cast to double #252
Conversation
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 to me, thank you!
@zacharyburnett can we squeeze this into 1.6.2? If not I'll need to update the changelog entry for this PR. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #252 +/- ##
==========================================
- Coverage 85.90% 85.18% -0.73%
==========================================
Files 35 35
Lines 6557 6797 +240
==========================================
+ Hits 5633 5790 +157
- Misses 924 1007 +83 ☔ View full report in Codecov by Sentry. |
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.
wow what a simple fix! I'll run these through the romancal regtests on github actions too when it's merged
Thanks! I'd hold off re-running them until @schlafly can update the truth files. The change here only breaks jenkins in the same way as the local tests were failing (so this PR will break 2 regtests in romancal until the truth files can be updated). |
The use of
sqrt
from libc results in a cast to/fromdouble
in the ols_cas22 ramp fitting (with jump detection). This can result in small numerical differences that lead to romancal regtests that passed on jenkins but failed locally (due to a single extra jump in one pixel).This PR switches
sqrt
forsqrtf
to keep the intermediate values asfloat
which leads to (at the moment) the same failure locally and on jenkins (wheretest_rampfit_step[spec_full]
fails with a single pixel difference):Regtest run here:
https://plwishmaster.stsci.edu:8081/job/RT/job/Roman-Developers-Pull-Requests/681/
The
test_resample
failure is unrelated and also occurs on main.Checklist
CHANGES.rst
(either inBug Fixes
orChanges to API
)