search

spacetelescope / stcal

https://stcal.readthedocs.io/en/latest/
Other
10 stars 32 forks source link

use sqrtf to avoid cast to double #252

Closed braingram closed 7 months ago

braingram commented 7 months ago

The use of sqrt from libc results in a cast to/from double 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 for sqrtf to keep the intermediate values as float which leads to (at the moment) the same failure locally and on jenkins (where test_rampfit_step[spec_full] fails with a single pixel difference):

    {'arrays_differ': {"root['roman']['data']": {'abs_diff': <Quantity 1.9270487 DN / s>,
                                                 'n_diffs': 1,
                                                 'worst_abs_diff': {'index': (2798,
                                                                              2758),
                                                                    'value': <Quantity 1.9270487 DN / s>},

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

braingram commented 7 months ago

@zacharyburnett can we squeeze this into 1.6.2? If not I'll need to update the changelog entry for this PR.

codecov[bot] commented 7 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 85.18%. Comparing base (41259f3) to head (af5aefb). Report is 11 commits behind head on main.

Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

braingram commented 7 months ago

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).