RCAL-896: update TweakRegStep to use common code from stcal

[mairanteodoro]

Resolves RCAL-896

The TweakRegStep class has been refactored to improve the handling of image alignment processes. This is the first step towards trying to make the class more maintainable on the long term.

The refactoring includes the removal of unused imports, restructuring of methods for better readability, and the introduction of new helper functions to manage catalog parsing and image alignment. The test suite in test_tweakreg.py has been updated to reflect these changes, with several tests removed or modified to align with the new implementation.

Regression tests
All tests are passing.

• https://github.com/spacetelescope/RegressionTests/actions/runs/10924983424

Checklist

• added entry in CHANGES.rst under the corresponding subsection
• updated relevant tests
• updated relevant documentation
• updated relevant milestone(s)
• added relevant label(s)
• ran regression tests, post a link to the Jenkins job below. How to run regression tests on a PR

[codecov]

Codecov Report

Attention: Patch coverage is 84.26966% with 14 lines in your changes missing coverage. Please review.

Project coverage is 78.67%. Comparing base (96af8bb) to head (59b490e).

Files with missing lines	Patch %	Lines
romancal/tweakreg/tweakreg_step.py	84.26%	14 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1395      +/-   ##
==========================================
+ Coverage   78.52%   78.67%   +0.14%     
==========================================
  Files         117      117              
  Lines        7842     7728     -114     
==========================================
- Hits         6158     6080      -78     
+ Misses       1684     1648      -36     

Flag	Coverage Δ		*Carryforward flag
nightly	62.26% <ø> (-0.03%)	⬇️	Carriedforward from ce4fe62

*This pull request uses carry forward flags. Click here to find out more.

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

[braingram]

Overall looks good. I left a few comments and suggestions. Thanks for working on this!

[braingram]

It looks like in the previous code this step exited when a single non-WFI_IMAGE exposure type was found. With this PR it looks like the step would attempt to process the WFI_IMAGE exposures. However I think this would lead to a bug where the imcats appended here are assumed to have matching indices to the models. That assumption won't hold for an association with non-image and image models. For example an association with "image, non-image image" with this PR I think would produce 2 imcats (for index 0 and index 2) but below would assign back to index 0 and 1 (since there are only 2 imcats).

Any objection to keeping the "exit if any non-wfi-image models"? Are there ever situations where this step will need to process non-WFI_IMAGE data (or associations that contain non-WFI_IMAGE and WFI_IMAGE models)?

[mairanteodoro]

The current implementation loops over the input, and if the exposure type is a non-WFI_IMAGE, we set meta.cal_step to "SKIPPED" and then move on to the next input. At the end, it returns a ModelLibrary object with the content updated.

[braingram]

I opened a PR with a test that fails with this PR. I don't know if this is a representative test (it has 2 WFI_IMAGE and 1 WFI_GRISM members) but I hope it illustrates the problem. The test fails on the call to del image_model.meta.source_detection["tweakreg_catalog"] because it attempts to delete tweakreg_catalog from the grism observation (and it didn't add tweakreg_catalog earlier in the step).
If it's not representative (if there's no expectation that an association will have grism and image members) than I think skipping tweakreg entirely (or throwing an error) on any non-image member (similar to the old behavior) is easier to implement. If handling these types of associations is required than the relationship between model index in the library and index in the imcats will need to be handled.

[schlafly]

Can you remind me, Mairan, if we need to support passing spectroscopic data to tweakreg at present? e.g., because the regtests send it, or because of the issue you identified in the ELP where it's passing more than it needs to. I agree with Brett that morally we should probably just error out immediately if we ever see spectroscopic data passed to tweakreg, but we don't need that in this PR necessarily.

[mairanteodoro]

Can you remind me, Mairan, if we need to support passing spectroscopic data to tweakreg at present?

Not to my knowledge. For example, the ELP pipeline behavior is to check for the exposure type, and if it is not a "WFI_IMAGE" then it sets meta.cal_step.tweakreg to "SKIPPED" and then added to a list that will be passed to TweakRegStep at the end. There's a note in the ELP code that reads:

# Now that all the exposures are collated, run tweakreg
# Note: this does not cover the case where the asn mixes imaging and spectral
#          observations. This should not occur on-prem

[mairanteodoro]

For now, TweakRegStep will always return all the datamodels passed on to it (even mixed exposure types). However, if the exposure is of type "WFI_IMAGE", the step will process it normally. Otherwise, it will set meta.cal_step="SKIPPED" and nothing else. But all the updated input files will still be returned to the caller.

Does that sound like a suitable workaround for handling mixed exposure type inputs?

[braingram]

Thanks for addressing my comments. I added a few more (final) ones:

• a minor change to one of the tests
• a minor change to the imports from stcal
• using imcat.meta["model_index"] instead of a dict containing an imcat and the model index

[braingram]

As the remaining comments are more style than substance I'll mark this as approved and feel free to make the changes if you agree or forge ahead otherwise.

Thanks!

[schlafly]

Thanks Mairan, looks good to me!