AL-852: GWCS inverse transform should respect its bounding box

[nden]

GWCS has a bug where the inverse transform does not respect the bounding_box. This leads to unexpected results that affect other applications. The GWCS bud is fixed in spacetelescope/gwcs#498
This PR fixes problems in jwst which surfaced when the bug was fixed.

Checklist for PR authors (skip items if you don't have permissions or they are not applicable)

• added entry in CHANGES.rst within the relevant release section
• updated or added relevant tests
• updated relevant documentation
• added relevant milestone
• added relevant label(s)
• ran regression tests, post a link to the Jenkins job below.
  How to run regression tests on a PR
• All comments are resolved
• Make sure the JIRA ticket is resolved properly

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 66.84%. Comparing base (e3d263f) to head (e3e8ab2).
Report is 2 commits behind head on main.

❗ There is a different number of reports uploaded between BASE (e3d263f) and HEAD (e3e8ab2). Click for more details.

HEAD has 1 upload less than BASE

Flag	BASE (e3d263f)	HEAD (e3e8ab2)
	2	1

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8554      +/-   ##
==========================================
- Coverage   76.81%   66.84%   -9.98%     
==========================================
  Files         496      376     -120     
  Lines       45610    37998    -7612     
==========================================
- Hits        35034    25398    -9636     
- Misses      10576    12600    +2024     

Flag	Coverage Δ
nightly	?

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

[emolter]

The failing oldestdeps tests appear to show that asdf needs a minimum of at least 3.3 now since that's what GWCS needs

[emolter]

ASDF still needs a bump to >=3.3.0

It looks like your auto-formatter touched CHANGES.rst, which is making the CI unhappy

Needs a changelog fragment

I forgot to remove two imports that are no longer used in test_outlier_detection.py, namely create_fitswcs() and compute_s_region_imaging()

Other than those small things that are needed to make our CI pass, this looks good to me.

[nden]

Regression test run: https://github.com/spacetelescope/RegressionTests/actions/runs/12395581275/job/34601785739

MIRI LRS differences are expected, since the bounding_box has changed.
NRS differences are: 1 px in one case and 4 px in the other.
NRS moving target differences are bigger, worth looking into ths.

[emolter]

NRS moving target differences are bigger, worth looking into ths.

Without doing too deep a dive, just plotting some of the outputs, it looks to me like as we guessed in stand-up there are just a few more flagged pixels in the data which are then propagating to the output spectra and it's not much to worry about. But I'll let Melanie have the final word on this one

[WilliamJamieson]

These changes look good for me other than the comments made by @emolter which look relatively trivial.

Does this warrant a change log fragment?

[melanieclarke]

Thanks Ned. Specifically, it is these pixels at the upper edge of the second SCI extension image (top: branch test output crf file, bottom: truth):

The border pixels used to be marked as outliers, and now are not. I suspect it's just that the edge of the array shifted a little due to the changes, and these pixels were on a threshold. It also appears to be architecture dependent - on my Mac, the regtest outputs show no difference from truth for this test, on this branch.

I agree it's not a cause for concern. There is no impact on the output spectra.

[mcara]

I thought deepcopying a WCS is cpu-intense. Wouldn't it be better to you try-finally to turn it off/on? Still, I think @emolter 's comment about turning this off may be hiding some other issue is relevant. That is, if we do not disable bbox on the inverse, will the custom WCS test (as modified here) still work?

[mcara]

I think most changes are reasonable except for the custom wcs test where output shape is increased to 10k pixels. I believe the original test does not work because of innacurate crval and that issue about the bbox calculation in stcal.

[mcara]

I think test_custom_wcs_resample_imaging needs better parameters that do not require these large output shapes. I also think that we need to investigate why it is necessary to disable bounding box for pixmap computations. However, this should prevent merging this PR as it allows the pipeline to work as before the major GWCS change.

[emolter]

Attempting to summarize a conversation between @mcara, @braingram, @tapastro, @perrygreenfield and I this morning.

This PR fixes multiple issues.
We are happy with the changes that set with_bounding_box=False.

The other change is to unset the bounding_box before calling wcs.world_to_pixel_values. This reproduces the old behavior of the code, so we think it's ok to merge it for now. However, it basically bypasses/hides a bug where the pixmap is sometimes all NaNs, stemming from the fact that the bounding_box is not updated to account for the value of crpix when it is nonzero.

The latter bug is fixed by spacetelescope/stcal#326, but that PR is currently causing some unexpected Romancal failures. Additional discussion is needed before merging that one.

The proposed plan of action is to:

1. Ensure the latest regtest failures on this PR are indeed expected - we are a little bit suspicious of the MIRI LRS s_region differences, because they look large
2. Merge this PR basically as-is
3. Finish build 11.2 - all the other changes can wait
4. Finalize JP-3824: Fix a bug in wcs_from_sregions when crpix is provided stcal#326, then see what happens if we revert the unsetting of bounding_box in this PR after that fix is merged
5. Decide what should happen with Fix issues in outlier det. due to GWCS.inverse enforcing bbox stcal#324, which also involves bypassing the bounding_box in various functions. It is not clear to me whether this would be necessary if the other bug were fixed.

Also tagging @melanieclarke to see if that sounds good

[emolter]

I'm happy with this given that we think we know the cause of the LRS bounding_box differences