AL-837: Calculate output WCS for resample from already-known s_region

[emolter]

Resolves AL-837

This PR simplifies the calculation of the output WCS for resample. Previously, the wcs_from_footprints function accepted a list of WCS objects, one for each input model, and computed the footprints from those WCS objects to pass into _calculate_new_wcs(). However, computing the footprints here is unnecessary because that information is already stored in the S_REGION keyword. This PR makes wcs_from_footprints take in an actual footprint instead, in the form of either a numpy array or an S_REGION string.

This PR will require small changes to both jwst and romancal, both of which are using the wcs_from_footprints function. I'm not sure there's a way to avoid that. One other non-hidden function changed, compute_fiducial, but this is not used for JWST or Romancal - both repositories have their own copy of a function with that name.

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 below for change types)
  • run regression tests with this branch installed ("git+https://github.com/<fork>/stcal@<branch>")
    • jwst regression test
    • romancal regression test

news fragment change types...

• changes/<PR#>.apichange.rst: change to public API
• changes/<PR#>.bugfix.rst: fixes an issue
• changes/<PR#>.general.rst: infrastructure or miscellaneous change

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.33%. Comparing base (60bd3b8) to head (1d6f0a0).
Report is 5 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #307      +/-   ##
==========================================
+ Coverage   86.21%   86.33%   +0.12%     
==========================================
  Files          47       49       +2     
  Lines        8812     8894      +82     
==========================================
+ Hits         7597     7679      +82     
  Misses       1215     1215              

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

[emolter]

JWST regression tests started here

[emolter]

tracking down JWST regression test failures now

[mairanteodoro]

romancal's issue 1455 needs to be resolved before this PR, since it seems that TweakRegStep is only updating the WCS but not s_region.

[emolter]

@mairanteodoro good to know. When you do update s_region, it might be good to be aware of this comment. The regression test failures for JWST come from the fact that wcs.footprint() and S_REGION differ by half a pixel due to JWST's use of center=True in compute_s_region_imaging()

[braingram]

Does this have a corresponding jwst PR? It is showing test failures in both jwst and romancal. Based on the comment from @mairanteodoro this can't go in until after romancal is updated to compute s_region. @emolter would you update this PR to avoid the breaking changes to the API that are causing the downstream failures?

[emolter]

corresponding JWST PR is here. I know this needs to wait for that Romancal change, and then someone (it could be me) will need to make a similar PR for Roman as JWST.

It's a bit pointless to "avoid" the breaking changes according to my understanding of what is going on here. One of the main reasons we want to do this is so that we can avoid passing in a WCS object for every input model, preferring to pass in s_region strings instead. And that's the part of the API that is changing.

I suppose it would be possible to support both, i.e., allow wcs_from_footprints() to take in either one, and I'll do that if you think it's a good idea, but I personally don't think it's necessarily a good idea.

[braingram]

corresponding JWST PR is here. I know this needs to wait for that Romancal change, and then someone (it could be me) will need to make a similar PR for Roman as JWST.

It's a bit pointless to "avoid" the breaking changes according to my understanding of what is going on here. One of the main reasons we want to do this is so that we can avoid passing in a WCS object for every input model, preferring to pass in s_region strings instead. And that's the part of the API that is changing.

I suppose it would be possible to support both, i.e., allow wcs_from_footprints() to take in either one, and I'll do that if you think it's a good idea, but I personally don't think it's necessarily a good idea.

I think this would be easier to manage and review if a new function was added wcs_from_s_regions. That wouldn't break jwst or romancal right? That would also mean this PR could be merged at any point without having to coordinate it with simultaneous merging of this a jwst and a romancal PR.

I believe stcal is semantically versioned and as wcs_from_footprints is a documented part of the public API any breaking change should be preceded by a deprecation warning and the change made in a major version change.

[emolter]

Should we deprecate wcs_from_footprints? It will no longer be used by either jwst or roman, and while stcal is supposed to be generally applicable, I don't think we really have any other users. So I'd be in favor of removing it just to avoid having to maintain it. Plus, I dislike the name, as it doesn't take in footprints, it takes in a WCS list.

@kmacdonald-stsci might be the best person to answer this. I think since romancal is still using it deprecating it would allow romancal to continue to work (with the warning, which I think isn't turned into an error for romancal). Once romancal no longer uses it the function could be removed (with a major version bump to stcal).

[emolter]

deprecating it would allow romancal to continue to work (with the warning, which I think isn't turned into an error for romancal)

It appears you are correct that romancal doesn't turn the deprecation warning into an error. Based on the logs of the passing romancal downstream tests, the warning message displays properly but the test still passes.

[emolter]

new JWST regtests with this branch here

Edit: Most of the regression test failures look like they're caused by small numerical differences: this is the case for all the test_nircam_image, test_miri_image, and test_fgs_image2 failures. I now see though that there are larger differences for nircam and miri coron3 i2d products as well as nircam image moving target i2d products - I will try to figure out the cause of those and ensure they are expected.

Edit 2: These failures are happening because the input and truth files to these tests were never updated after spacetelescope/jwst#8897. The input files are all Level 2b products, so the regression tests never call assign_wcs on them. They are failing now because the coroon3 or image3 pipeline is attempting to compute the footprint from their (incorrect) s_regions instead of their (correct) wcs.footprints. I will try updating the s_regions of the input and truth files using the new update_s_region_imaging script and see if this fixes the problem.

Edit 3: I confirmed that this is the problem. By updating the s_regions of the input files, this is fixed locally. There are still regression test failures, but they look more like the ones that are currently happening for e.g. test_nircam_image.py. So this will have to be fixed by first changing the s_regions of the input files, then re-running the regtests and ok-ifying the results. Round of regtests on jwst/main and stcal/main with the input file s_regions updated are here; failures related to s_regions of truthfiles are expected.

Edit 4: rerunning regtests for emolter/AL-837 after ok-ify

As an aside, this seems to expose a weakness in the way we structure certain regtests: tests that start halfway through processing with e.g. Level 2b products are prone to having their inputs get out-of-date. I wonder if this should be fixed, and I can see both sides because the tests certainly run much faster without having to re-run the detector1pipeline all the time.

[nden]

I think we need to come up with a consistent naming of fiducial and other user facing functions but this PR touches those only in private functions so this is for a future PR.

[mairanteodoro]

Roman's TweakRegStep update for review: spacetelescope/romancal#1484