Skip to content

AL-1042: Use roman-specific tweakwcs corrector#2219

Merged
schlafly merged 12 commits into
spacetelescope:mainfrom
mcara:move-corrector-to-roman-pipeline
Jun 3, 2026
Merged

AL-1042: Use roman-specific tweakwcs corrector#2219
schlafly merged 12 commits into
spacetelescope:mainfrom
mcara:move-corrector-to-roman-pipeline

Conversation

@mcara

@mcara mcara commented Mar 6, 2026

Copy link
Copy Markdown
Member

Resolves AL-1042

This PR switches correctors used in tweakreg from a jwst corrector to a dedicated roman-specific corrector. This PR depends on spacetelescope/tweakwcs#243 and spacetelescope/stcal#529 being merged and teakwcs version 0.9.0 be released. Unit tests will not pass unless we modify pyproject.toml to use those branches of stcal and tweakwcs.

Tasks

  • request a review from someone specific, to avoid making the maintainers review every PR
  • add a build milestone, i.e. 24Q4_B15 (use the latest build if not sure)
  • Does this PR change user-facing code / API? (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
    • update or add relevant tests
    • update relevant docstrings and / or docs/ page
    • start a regression test and include a link to the running job (click here for instructions)
      • Do truth files need to be updated ("okified")?
        • after the reviewer has approved these changes, run okify_regtests to update the truth files
  • if a JIRA ticket exists, make sure it is resolved properly

@mcara mcara self-assigned this Mar 6, 2026
@mcara mcara requested review from a team and mairanteodoro as code owners March 6, 2026 17:43
@github-actions github-actions Bot added the dependencies Pull requests that update a dependency file label Mar 6, 2026
@mcara

mcara commented Mar 6, 2026

Copy link
Copy Markdown
Member Author

Comment thread changes/2219.tweakreg.rst Outdated
@codecov

codecov Bot commented Mar 6, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.99%. Comparing base (2640904) to head (c66b625).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2219   +/-   ##
=======================================
  Coverage   80.99%   80.99%           
=======================================
  Files         159      159           
  Lines        9502     9503    +1     
=======================================
+ Hits         7696     7697    +1     
  Misses       1806     1806           

☔ 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

mcara commented Mar 7, 2026

Copy link
Copy Markdown
Member Author

Although regression tests fail, it is actually just one unit test that is failing and, based on the line numbers in the traceback, it looks like unit tests (when run as part of the entire regression test) do not use install overrides and so they do not use required package versions by tweakwcs and stcal.

@mcara

mcara commented Mar 7, 2026

Copy link
Copy Markdown
Member Author

Last commit (only temporary, to illustrate the last point) sets dependency to spacetelescope/stcal#529 and the regression tests with this commit are: https://github.com/spacetelescope/RegressionTests/actions/runs/22793185624

@mcara

mcara commented Mar 11, 2026

Copy link
Copy Markdown
Member Author

Tests are going to fail until the stcal and tweakreg PRs are merged. In a similar PR for romancal, once I modified pyproject.toml thests passes (they were failing with the same error): #2219

@mcara

mcara commented Mar 11, 2026

Copy link
Copy Markdown
Member Author

If you want, I can make a temporary commit to make them pass (it will need to be dropped before merging)

@schlafly

Copy link
Copy Markdown
Collaborator

I'm okay with either a temporary commit or a link to passing regression tests. The approach you're taking here looks reasonable to me and I'm happy to move forward with it, pending your deciding which of the various options is the best. Thanks!

@mcara mcara marked this pull request as draft March 11, 2026 19:44

@mairanteodoro mairanteodoro left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! Thanks, Mihai!

I can approve once all the tests are passing.

@mcara mcara marked this pull request as ready for review March 18, 2026 13:48
@mcara mcara force-pushed the move-corrector-to-roman-pipeline branch from 03401d0 to 15f46ca Compare March 19, 2026 07:14

@PaulHuwe PaulHuwe left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - Thanks!

@mcara mcara force-pushed the move-corrector-to-roman-pipeline branch 3 times, most recently from f759609 to 8f931b7 Compare May 15, 2026 12:50

@schlafly schlafly left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor comment inline. In general I'm happy to approve this conditionally upon the dependencies getting merged.

If I understood correctly, the stcal release (that will pull in the tweakwcs release) is still compatible with the old code, so that release can happen without needing to bump stcal's major version number, and then this PR can go in?

Comment thread romancal/tweakreg/tweakreg_step.py
@mcara

mcara commented May 26, 2026

Copy link
Copy Markdown
Member Author

One minor comment inline. In general I'm happy to approve this conditionally upon the dependencies getting merged.

If I understood correctly, the stcal release (that will pull in the tweakwcs release) is still compatible with the old code, so that release can happen without needing to bump stcal's major version number, and then this PR can go in?

Yes, stcal PR can be merged independently and it should be backwards compatible as these tests show: spacetelescope/stcal#529 (comment)

@mcara mcara force-pushed the move-corrector-to-roman-pipeline branch from da02dbc to ed74415 Compare May 26, 2026 05:54

@mairanteodoro mairanteodoro left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that all the other PRs connected with this one have been merged. Thanks, @mcara!

@mcara

mcara commented May 27, 2026

Copy link
Copy Markdown
Member Author

Do not merge this PR until there is a new release of stcal and pyproject.toml is updated accordingly.

@mcara

mcara commented May 27, 2026

Copy link
Copy Markdown
Member Author

JWST PR does not need stcal release or tweakwcs release. This PR needs stcal.

@mcara mcara force-pushed the move-corrector-to-roman-pipeline branch from ed74415 to 19e0909 Compare May 28, 2026 04:33
@schlafly

Copy link
Copy Markdown
Collaborator

Since the stcal PR is in, normally we were point pyproject.toml to point to stcal github main now, and then this could be merged?

@mcara

mcara commented May 28, 2026

Copy link
Copy Markdown
Member Author

Since the stcal PR is in, normally we were point pyproject.toml to point to stcal github main now, and then this could be merged?

Oh, if you are OK to pointing to stcal@main that should work.

@mcara mcara force-pushed the move-corrector-to-roman-pipeline branch from 3467023 to 76b8889 Compare May 28, 2026 19:32
@mcara mcara force-pushed the move-corrector-to-roman-pipeline branch from 76b8889 to 139fdc6 Compare May 28, 2026 19:37
@mcara

mcara commented May 28, 2026

Copy link
Copy Markdown
Member Author

ready to merge

@mcara

mcara commented May 28, 2026

Copy link
Copy Markdown
Member Author

Started a new run of regression tests: https://github.com/spacetelescope/RegressionTests/actions/runs/26597890960

@mcara

mcara commented May 28, 2026

Copy link
Copy Markdown
Member Author

Don't know what happened with CI but regression tests pass.

@mcara

mcara commented May 28, 2026

Copy link
Copy Markdown
Member Author

Oh, I see, it's spacetelescope/crds#1207 that prevents CI from starting. Anyway, regression tests pass so it should be OK to merge this.

@schlafly schlafly left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@schlafly schlafly merged commit b6b035b into spacetelescope:main Jun 3, 2026
48 of 52 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dependencies Pull requests that update a dependency file tweak_reg tweakreg

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants