-
Notifications
You must be signed in to change notification settings - Fork 169
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
JP-3624: Fix a source of errors in resampled spec WCS #8511
JP-3624: Fix a source of errors in resampled spec WCS #8511
Conversation
ef531f5
to
2d65aa2
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #8511 +/- ##
========================================
Coverage 57.97% 57.98%
========================================
Files 387 388 +1
Lines 38830 38930 +100
========================================
+ Hits 22513 22572 +59
- Misses 16317 16358 +41 ☔ 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.
Single-line code change looks fine to me. Are the regression test differences for test_miri_lrs_slit_spec2
expected? There appear to be changes in a majority of the pixels, and some of them are several percent. Also, I assume you have already ensured that this change does indeed allow the exposures mentioned in the original ticket to be combined? If so, it might be helpful to add a note to the ticket to that effect.
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.
Agree with @emolter that I'd very much like to see a visual before/after comparison of an example where it makes only a slight difference, as well as the example observation that was so far off as to trigger the ticket in the first place.
56f965f
to
bf1aeec
Compare
Co-authored-by: Ned Molter <[email protected]>
58ca7b7
to
2a90f8d
Compare
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.
Examples of new results that are provided in the JP ticket look really good. I approve.
Resolves JP-3624
Closes #8485
Closes #8484
Checklist for PR authors (skip items if you don't have permissions or they are not applicable)
CHANGES.rst
within the relevant release sectionHow to run regression tests on a PR
Regression test: https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/1479/