-
Notifications
You must be signed in to change notification settings - Fork 28
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
Replace create_s_region with stcal's compute_s_region_keyword. #1493
Conversation
9e689b2
to
964bade
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1493 +/- ##
==========================================
- Coverage 76.20% 76.19% -0.01%
==========================================
Files 115 115
Lines 7628 7626 -2
==========================================
- Hits 5813 5811 -2
Misses 1815 1815 ☔ View full report in Codecov by Sentry. |
Tagging @emolter since I can't find him on the list of reviewers. |
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
9181e72
to
5b417f9
Compare
Judging by the regtest changes, it looks like stcal's variant uses less precision. It looks like this is still ~3 uas, so I have trouble imagining we care about this accuracy for s_region. But is it known or expected that we changed the precision here? |
See https://github.com/spacetelescope/stcal/blob/cb96233b3bf00dbd003c5652bd8c6b928857266b/src/stcal/alignment/util.py#L868 |
Thanks, okay. Then I'll treat this as expected and reasonable and approve, thanks! |
5b417f9
to
c2e6888
Compare
Resolves RCAL-948
Partially resolves #1466
This PR updates
romancal
to reduce duplicate code by usingstcal
'scompute_s_region_keyword()
.Regression test
Tasks
24Q4_B15
(use the latest build if not sure)no-changelog-entry-needed
)changes/
:echo "changed something" > changes/<PR#>.<changetype>.rst
(see below for change types)docs/
pageokify_regtests
to update the truth filesnews fragment change types...
changes/<PR#>.general.rst
: infrastructure or miscellaneous changechanges/<PR#>.docs.rst
changes/<PR#>.stpipe.rst
changes/<PR#>.associations.rst
changes/<PR#>.scripts.rst
changes/<PR#>.mosaic_pipeline.rst
changes/<PR#>.patch_match.rst
steps
changes/<PR#>.dq_init.rst
changes/<PR#>.saturation.rst
changes/<PR#>.refpix.rst
changes/<PR#>.linearity.rst
changes/<PR#>.dark_current.rst
changes/<PR#>.jump_detection.rst
changes/<PR#>.ramp_fitting.rst
changes/<PR#>.assign_wcs.rst
changes/<PR#>.flatfield.rst
changes/<PR#>.photom.rst
changes/<PR#>.flux.rst
changes/<PR#>.source_detection.rst
changes/<PR#>.tweakreg.rst
changes/<PR#>.skymatch.rst
changes/<PR#>.outlier_detection.rst
changes/<PR#>.resample.rst
changes/<PR#>.source_catalog.rst