AL-850: adding tweakreg into stcal

[emolter]

Resolves AL-850 Partially resolves JP-3668

This PR moves the majority of the tweakreg functionality from JWST into stcal, such that it can be used by both Roman and JWST without needing to be maintained in both places. This relates to the alignment between images and to an absolute source catalog only, but not to finding sources within images - this is handled differently by the two observatories, and is split into its own step for Roman.

Several WCS-related utilities have also been moved into alignment or modified therein. These should also eventually be called by JWST and Roman from stcal and removed in those repositories.

Checklist

• [x] added entry in CHANGES.rst (either in Bug Fixes or Changes to API)
• [x] updated relevant tests
• [x] updated relevant documentation
• [ ] updated relevant milestone(s)
• [x] added relevant label(s)

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 85.60794% with 58 lines in your changes missing coverage. Please review.

Project coverage is 84.01%. Comparing base (5772579) to head (37ea975). Report is 196 commits behind head on main.

Files with missing lines	Patch %	Lines
src/stcal/tweakreg/tweakreg.py	68.64%	37 Missing :warning:
src/stcal/alignment/util.py	79.48%	8 Missing :warning:
src/stcal/tweakreg/utils.py	63.15%	7 Missing :warning:
src/stcal/tweakreg/astrometric_utils.py	90.00%	6 Missing :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #267 +/- ## ========================================== + Coverage 83.79% 84.01% +0.22% ========================================== Files 35 39 +4 Lines 6998 7345 +347 ========================================== + Hits 5864 6171 +307 - Misses 1134 1174 +40 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[braingram]

Once the unit tests are sorted would you run the jwst and romancal regtests with this PR?

The romancal unit tests are currently showing failures in part due to the change in signature for wcs_from_footprints: https://github.com/spacetelescope/stcal/actions/runs/9975888017/job/27566761372?pr=267#step:10:10078

[emolter]

Once the unit tests are sorted would you run the jwst and romancal regtests with this PR?

The romancal unit tests are currently showing failures in part due to the change in signature for wcs_from_footprints: https://github.com/spacetelescope/stcal/actions/runs/9975888017/job/27566761372?pr=267#step:10:10078

Yeah, I definitely expect failures in both jwst and romancal because of the call signature changes that were necessary to avoid adding datamodel as a dependency to stcal. I am not sure whether I have permission to start regtest runs for Roman, and I'm not sure whether it's my responsibility or the responsibility of someone else to modify romancal to account for these changes in stcal. But I will go ahead and start another JWST regtest run based on this PR branch and the corresponding JWST PR branch.

[emolter]

The mypy checking suggested by @braingram raised an additional issue, which is that wcsinfo isn't actually a dict, but an stdatamodels.properties.ObjectNode, and its attributes are called by many of these functions. I see two ways to fix this:

1. change all instances of wcsinfo.property to wcsinfo["property"] and then pass wcsinfo.instance into all the functions that currently require wcsinfo, since wcsinfo.instance is a dict.
2. write an adapter class that takes in an Any type and attempts to turn it into a wcsinfo dict.

I'm tempted to go with the former. It will require even more changes to the jwst and romancal repositories, but those changes are already needed to do away with the SupportsDataWithWCS protocol. Thoughts?

[mairanteodoro]

The mypy checking suggested by @braingram raised an additional issue, which is that wcsinfo isn't actually a dict, but an stdatamodels.properties.ObjectNode, and its attributes are called by many of these functions.

@emolter I'm not sure about JWST, but for Roman meta.wcsinfo is of type roman_datamodels.stnode.Wcsinfo and it has a method .to_flat_dict() to parse it into a flat dict.

[emolter]

The mypy checking suggested by @braingram raised an additional issue, which is that wcsinfo isn't actually a dict, but an stdatamodels.properties.ObjectNode, and its attributes are called by many of these functions.

@emolter I'm not sure about JWST, but for Roman meta.wcsinfo is of type roman_datamodels.stnode.Wcsinfo and it has a method .to_flat_dict() to parse it into a flat dict.

thanks, that exists in jwst as well. Before, the SupportsDataWithWCS class required the to_flat_dict method, but it sounded like people were interested in removing it. Instead of doing that, I guess we could go for the option of only applying SupportsDataWithWCS to wcsinfo objects, not to (for lack of a better word) "datamodel-like" objects

[emolter]

Ok, I put a proposed fix to the wcsinfo typing issue into the most recent push. In short, I wrote a protocol for the wcsinfo object specifying that it needs a .instance attribute that converts it to a dict, as well as attributes corresponding to the expected metadata fields (e.g. roll_ref, v3yangle, etc.). All the methods that take in a wcsinfo object now also accept a dict.

I chose this design because I think the protocol is specific enough that a different observatory outside the stsci umbrella could straightforwardly adapt it to their needs (That is the original idea of stcal, right?) while the romancal and jwst repos won't need to pass in wcsinfo.instance or similar every time they want to use one of the utility functions.

I was wrong that wcsinfo in the jwst repo has a to_flat_dict() method, so I am using .instance instead - is that going to work for Roman?

I also updated all the alignment utils to have type hints. While not specifically requested in this PR, I think it made sense given the current typing discussion. It would be good if someone could review those.

I think all other comments have been addressed, besides the one about the data file creation in the tests/ folder.

Let me know what you think.

Note that as Brett mentioned, merging this is going to break romancal and jwst main branches because we are removing the (implicit) dependency on the datamodel structure, which has effects beyond only tweakreg. I hope that's ok. If so, a coordinated release is probably necessary - not sure how that works.

edit: of course this broke the docs build again and I didn't check before pushing! working on that now

[braingram]

Yeah, I definitely expect failures in both jwst and romancal because of the call signature changes that were necessary to avoid adding datamodel as a dependency to stcal. I am not sure whether I have permission to start regtest runs for Roman, and I'm not sure whether it's my responsibility or the responsibility of someone else to modify romancal to account for these changes in stcal. But I will go ahead and start another JWST regtest run based on this PR branch and the corresponding JWST PR branch.

I think it's best if we address the known failures first.

As the changes will constitute a breaking change we'll need to consider:

• updating pins in jwst/romancal (more on this below)
• should this trigger a stcal 2.x.x

The currently release romancal does not have an upper pin for stcal: https://github.com/spacetelescope/romancal/blob/008e9ad726be3c84aa54c6979060d1793e7a5667/pyproject.toml#L31 jwst has one set to "<1.8.0": https://github.com/spacetelescope/jwst/blob/d4bd628cf527ae3b0bd33a191cdb778f0f532293/pyproject.toml#L37

This PR should at least trigger 1.8.0 (to prevent breaking jwst). romancal is in a worse state (I think that's more a sign that we should always pin stcal in romancal). Working with what we have I'm wondering if it makes more sense to deprecate the old functions if the old signatures can't be supported.

Do you think the api changes that are breaking romancal are required?

[mcara]

I also think that the original approach with the interface was not bad actually. Many years ago the thought was that NDData would be a data structure used by astronomical data software and provide a common interface definition that would hold data/error/variance/etc. arrays and WCS information. I think having a common base class (not necessarily NDData) for JWST and Roman that would provide a common interface would be great.

[emolter]

@braingram sorry for making you ask twice about mypy and type hits. It may not look like it (apparently it doesn't), but I did look through those, I just didn't see many of them as problems. However I now realize that some small changes could fix these:

• Skipping analyzing "astropy": module is installed, but missing library stubs or py.typed marker. These, and similar, are (to my understanding) beyond my control and have to do with the way those packages are themselves set up. This represents a large fraction of the errors. If there's a way to get mypy to ignore those, that would probably be helpful.
• Value of type "dict[Any, Any] | Wcsinfo" is not indexable. These were due to the fact that I didn't type hint the .instance function definition in the Wcsinfo protocol. This has been fixed.
• alignment/util.py:196: error: Incompatible types in assignment (expression has type "tuple[tuple[Any, Any], ...]", variable has type "list[tuple[Any, Any]]"). This was due to an unnecessary explicit assignment of output_bounding_box to type tuple when it seems to be fine for all purposes to have either a list or tuple. There were other similar errors that could be fixed by making all type hints consistently Sequence.
• alignment/util.py:278: error: "None" object is not iterable. and some similar errors related to allowing None as a type. These were caused by problems in function logic that could theoretically allow a NoneType to pass through if var is None kinds of checking (although in some cases mypy seemed not to follow the logic quite right)

In any case, the only mypy issues left now should be due to missing library stubs.

[braingram]

No worries and thanks for rerunning them. I'm also seeing 34 errors (the same as main) after the changes you've added. Thanks for the details on the failures. I'm certainly new to type hints and this PR has taught me a lot.

[emolter]

I put some lines of mypy configuration into pyproject.toml, and there are now zero failures with mypy using that configuration.

For some reason, mypy wasn't picking up the configuration unless I specified explicitly mypy filename --config-file path/to/pyproject.toml but I assume that if a mypy check is ever added to our CI workflow it will pick up the proper configuration in pyproject.toml

[braingram]

I put some lines of mypy configuration into pyproject.toml, and there are now zero failures with mypy using that configuration.

For some reason, mypy wasn't picking up the configuration unless I specified explicitly mypy filename --config-file path/to/pyproject.toml but I assume that if a mypy check is ever added to our CI workflow it will pick up the proper configuration in pyproject.toml

Thanks! I opened a PR against your branch to run mypy in the CI: https://github.com/emolter/stcal/pull/1

[emolter]

Do not merge at this time. somehow one of the last small changes broke s_region values in jwst. looking into it

[emolter]

Long story short, isinstance(wcsinfo, Wcsinfo) returns False for JWST wcsinfo objects, of type ObjectNode. However, if I do

for attr in ["ra_ref", "dec_ref", ...]:
    assert hasattr(wcsinfo, attr)

all of the assert statements pass. The documentation and other snooping I've done seem to indicate that a runtime_checkable protocol should be basically shorthand for all the hasattr statements, so I'm pretty confused about what's happening there. My only idea (based on PEP 544) is that these ObjectNode objects are not initialized until called explicitly, but I don't know enough about those objects to be sure.

I tried updating all the JWST code to pass in wcsinfo as a dict in all cases (using wcsinfo.instance), but this leads to the issue that updating keywords in the wcsinfo object via e.g. update_s_region_keyword becomes cumbersome and IMO unnecessarily verbose (it becomes necessary to reassign all the values from the dict to the wcsinfo object after the fact).

While perhaps not perfect, changing back to if not isinstance(wcsinfo, dict): achieves the desired behavior. While technically the not would make possible passing any other type through, the type hint (still using the custom Wcsinfo protocol but no longer runtime checkable) specifies what attributes a wcsinfo object needs to have for these routines to run properly. Let me know if that solution sounds ok to you @braingram.

edit: still please do not merge until fully tested with jwst regtests

[emolter]

Ok, all the regression tests for the JWST repo are now passing. It may be prudent to wait for more approvals of that PR, just in case changes are requested that require updating the stcal side for some reason. Other than that, this is ready to merge. @zacharyburnett what is the status of the romancal release - is it now safe to merge into stcal? @braingram are you happy with the changes now, and if so, can you remove the "changes requested" status?