Skip to content

Prepare for moving construct_wcs_corrector out to pipelines#529

Open
mcara wants to merge 3 commits intospacetelescope:mainfrom
mcara:move-corrector-to-pipelines-step1
Open

Prepare for moving construct_wcs_corrector out to pipelines#529
mcara wants to merge 3 commits intospacetelescope:mainfrom
mcara:move-corrector-to-pipelines-step1

Conversation

@mcara
Copy link
Member

@mcara mcara commented Mar 6, 2026

This PR is the first step of moving construct_wcs_corrector to pipelines. This is the approach that I favor for fixing #528 in conjunction with a new release (0.9.0) of tweakwcs after spacetelescope/tweakwcs#243 is merged.

Associated Prs:
spacetelescope/romancal#2219
spacetelescope/jwst#10306

Tasks

  • update or add relevant tests
  • update relevant docstrings and / or docs/ page
  • Does this PR change any API used downstream? (if not, label with no-changelog-entry-needed)
    • write news fragment(s) in changes/: echo "changed something" > changes/<PR#>.<changetype>.rst (see changelog readme for instructions)
    • if your change breaks existing functionality, also add a changes/<PR#>.breaking.rst news fragment
  • run regression tests with this branch installed ("git+https://github.com/<fork>/stcal@<branch>")

@mcara mcara requested review from PaulHuwe and braingram March 6, 2026 15:20
@mcara mcara self-assigned this Mar 6, 2026
@mcara mcara requested a review from a team as a code owner March 6, 2026 15:20
@mcara
Copy link
Member Author

mcara commented Mar 6, 2026

Regression test for JWST: https://github.com/spacetelescope/RegressionTests/actions/runs/22769717421
Romancal should pass if JWST passes - the changes are minor and affect both pipelines in the same way.

@braingram
Copy link
Collaborator

Regression test for JWST: https://github.com/spacetelescope/RegressionTests/actions/runs/22769717421 Romancal should pass if JWST passes - the changes are minor and affect both pipelines in the same way.

This will also need a passing romancal regtest run.

Would you help me understand how this relates to #528 or spacetelescope/tweakwcs#243

What is the plan here? Is this PR ready independent of spacetelescope/tweakwcs#243?

@mcara
Copy link
Member Author

mcara commented Mar 6, 2026

What is the plan here? Is this PR ready independent of spacetelescope/tweakwcs#243?

Yes, it is independent, but the two follow-up PRs will depend on tweakwcs release.

@mcara
Copy link
Member Author

mcara commented Mar 6, 2026

@codecov
Copy link

codecov bot commented Mar 6, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.61%. Comparing base (c18565f) to head (c780362).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #529      +/-   ##
==========================================
- Coverage   91.93%   89.61%   -2.33%     
==========================================
  Files          62       62              
  Lines        8793     8790       -3     
==========================================
- Hits         8084     7877     -207     
- Misses        709      913     +204     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@mcara
Copy link
Member Author

mcara commented Mar 6, 2026

@braingram
Copy link
Collaborator

braingram commented Mar 18, 2026

Would it be easier (and equivalent) to just replace:

original_skycoord = corrector.meta["original_skycoord"]
separation = original_skycoord.separation(aligned_skycoord)

with:

 original_skycoord = _wcs_to_skycoord(corrector.original_wcs)
 separation = original_skycoord.separation(aligned_skycoord) 

and drop the handling of meta["original_skycoord"].

@mcara mcara force-pushed the move-corrector-to-pipelines-step1 branch from 57d26ba to 4cff4ce Compare March 18, 2026 14:51
@mcara
Copy link
Member Author

mcara commented Mar 18, 2026

Would it be easier (and equivalent) to just replace:

original_skycoord = corrector.meta["original_skycoord"]
separation = original_skycoord.separation(aligned_skycoord)

with:

 original_skycoord = _wcs_to_skycoord(corrector.original_wcs)
 separation = original_skycoord.separation(aligned_skycoord) 

and drop the handling of meta["original_skycoord"].

This is an excellent observation! I see no reason not to do this. Initially I wanted to eliminate WCS copying for performance reasons but I kept original_wcs and made a copy of it to be modified. The alternative is to drop deepcopying "original wcs" at cost of correctors having side-effects like modifying input arguments which I do not like.

@mcara mcara force-pushed the move-corrector-to-pipelines-step1 branch from be06f1a to 7c51aee Compare March 19, 2026 06:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants