-
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-3603: reorder tweakreg to reduce iterations through models #8424
JP-3603: reorder tweakreg to reduce iterations through models #8424
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #8424 +/- ##
==========================================
+ Coverage 57.93% 58.01% +0.08%
==========================================
Files 387 387
Lines 38839 38811 -28
==========================================
+ Hits 22502 22517 +15
+ Misses 16337 16294 -43 ☔ View full report in Codecov by Sentry. |
e77532e
to
3944532
Compare
b6f5959
to
fe25dc2
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.
These are my comments for now.
Thanks for the quick response! I'll leave my responses to the other comments. |
f9a27a9
to
f5d49e4
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.
To the best of my abilities, this looks fine.
Thanks for the review! Your input was helpful in understanding the code and improving this PR. |
f5d49e4
to
2cbf72b
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.
I have a few minor comments about the tests.
0681bb5
to
0256da3
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.
Thanks, @braingram! Looks good to me.
I only have a couple of suggestions for improvement:
- we should add docstring with the purpose of the test;
- we should break down
test_custom_catalog
into smaller pieces.
dc7e85f
to
87ad23e
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.
Thanks, @braingram! Looks good to me.
87ad23e
to
d38b076
Compare
re-enable skipped test and update it for new check
e708813
to
80bb840
Compare
@nden thanks for adding the regtest in #8477 @mcara I believe the changes in this PR should have little to no impact on #8476 but please let me know if this PR impacts that PR (or other planned work on tweakreg). @hbushouse (or anyone else) any objection to merging this now that it's re-opened after rebasing following the other tweakreg changes? |
@mcara @braingram @nden Is everyone agreed that this is ready to merge? No bad side effects with #8476 ? |
Looks OK to me. |
This PR reorganizes operations within the
tweakreg
step to work towards improving image3 performance.Changes include:
tweakreg_catalog
inModelContainer
(one of the reasons that class opens all models when constructed).wcs_from_footprints
on the in-memory wcs objects to compute the combined wcs for limiting the GAIA query (at the moment I also see no tests for this feature). This required a minor backwards-compatible change inassign_wcs.util
.align_wcs
requires that all correctors be in-memory this results in all input image_models also required to be in memory. Breaking this link will (eventually) allow the image_models to be loaded one-by-one instead of having them all in memory at the same time.TweakRegStep.process
into functions to improve readability (and make unit-testing easier)_write_catalog
to return a filename instead of modify the input modelThis PR adds a parametrized test for the custom catalog handling in tweakreg (marked as
slow
as it has many combinations, each taking ~2 seconds on my machine, this will still be run in the CI astox
passes the--slow
option topytest
).Regression tests run:
https://plwishmaster.stsci.edu:8081/blue/organizations/jenkins/RT%2FJWST-Developers-Pull-Requests/detail/JWST-Developers-Pull-Requests/1443/pipeline/195
with one unrelated failure which also appears on main:
https://plwishmaster.stsci.edu:8081/job/RT/job/JWST/2885/testReport/jwst.regtest/test_miri_lrs_slit_spec3/_stable_deps__test_miri_lrs_slit_spec3_user_wcs_shape1_s2d_/
Resolves https://jira.stsci.edu/browse/JP-3603
Closes #8434
Checklist for maintainers
CHANGES.rst
within the relevant release sectionHow to run regression tests on a PR