Prepare for moving construct_wcs_corrector out to pipelines

[mcara]

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>")
  • jwst regression test
  • romancal regression test

[mcara]

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]

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]

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]

Romancal tests: https://github.com/spacetelescope/RegressionTests/actions/runs/22771003096

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.80%. Comparing base (6ae62a7) to head (a04ec58).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #529   +/-   ##
=======================================
  Coverage   91.79%   91.80%           
=======================================
  Files          63       63           
  Lines        8858     8855    -3     
=======================================
- Hits         8131     8129    -2     
+ Misses        727      726    -1     

☔ 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]

My previous tests were set-up incorrectly. Starting new tests:

• JWST: https://github.com/spacetelescope/RegressionTests/actions/runs/22772788548
• Romancal: https://github.com/spacetelescope/RegressionTests/actions/runs/22772858867

[braingram]

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

stcal/src/stcal/tweakreg/tweakreg.py

Lines 352 to 353 in 0e218f9

	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]

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

stcal/src/stcal/tweakreg/tweakreg.py

Lines 352 to 353 in 0e218f9

	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]

One side-effect of #529 (comment) is that now downstream tests fail because this is a breaking change (well, affecting only unit tests - not the actual code).

[braingram]

One side-effect of #529 (comment) is that now downstream tests fail because this is a breaking change (well, affecting only unit tests - not the actual code).

Thanks. The failing jwst test looks like it's just testing the private stcal method _is_wcs_correction_small. I propose that we delete that test in jwst. Would you open a PR removing the test? One the test is removed the tests should pass with this PR and it can be approved and merged.

[mcara]

yes, I'll do that

[mcara]

@braingram Done in spacetelescope/jwst#10354

[PaulHuwe]

LGTM

[tapastro]

JWST has a build release this week - is it okay if we wait to merge this until after the release? I'm hoping to avoid any possible complications.

[mcara]

Regression tests:

JWST with this PR: https://github.com/spacetelescope/RegressionTests/actions/runs/25901248519
JWST with spacetelescope/jwst#10306 + this PR: https://github.com/spacetelescope/RegressionTests/actions/runs/25901866517

Roman with this PR: https://github.com/spacetelescope/RegressionTests/actions/runs/25902060576
Roman with spacetelescope/romancal#2219 and this PR: https://github.com/spacetelescope/RegressionTests/actions/runs/25902138588

[mcara]

@tapastro Can this PR be merged? Also, is it possible to make a micro stcal release (like 1.18.1) after this PR is merged or do we wait for the next minor release (1.19.0)?

[tapastro]

I have no objection to merging, but I want to better understand the impact of the changes here and in the JWST PR. Both require the tweakwcs release, but they do not depend on each other? If we make a release of stcal, do you expect that it will work with the last released version of jwst? We should test this to understand what version to release stcal under.

As I understand it, we can merge the two PRs in any order?

[mcara]

@tapastro No, tweakwcs PR was already merged but a new release has not yet been made. A new release of stcal would need a new release of tweakwcs (which can be made anytime). JWST/Roman PRs should be merged after this PR is merged. Dependencies - main branch vs minimum release - should be adjusted based on package release status.

This PR can be merged without merging JWST PR: it should not affect JWST at all. This is illustrated by the tests posted above which I copy here:

JWST (regression test) with this PR only (without the associated JWST PR): https://github.com/spacetelescope/RegressionTests/actions/runs/25901248519

[mcara]

I take

A new release of stcal would need a new release of tweakwcs (which can be made anytime)

back: the release already has been made - see https://github.com/spacetelescope/tweakwcs/releases/tag/0.9.0. Merging this PR should not affect neither JWST nor Roman pipelines. However, Roman would benefit only after the associated Roman PR is merged. Until then it will use JWST corrector. However, JWST should work with this PR and without merging spacetelescope/jwst#10306 (however, it should be merged when possible). This is proven by the regression test above. It works because for backwards compatibility I kept https://github.com/mcara/stcal/blob/23ee5ff0a1ffc690483cea90f66874b7b4b2c9f5/src/stcal/tweakreg/tweakreg.py#L383 in place. It will be removed in a future stcal release (via deprecation) once spacetelescope/jwst#10306 is merged and there is a new release of jwst and romancal. This entire transition was designed with maximum backwardss compatibility in mind.

[braingram]

Thanks!