JP-3824: Fix a bug in wcs_from_sregions when crpix is provided

[mcara]

Resolves JP-3824

wcs_from_sregions is used by make_output_wcs function in the JWST pipeline to compute output WCS of the resampled image for the ResampleStep . Currently wcs_from_sregions assigns a bounding box to the output WCS that is computed in the assumption that crpix is (0, 0) and it is not adjusted when crpix is provided by the user and when it is different from (0, 0). This causes unit test failures in JWST pipeline when run with master branch of gwcs since now inverse WCS transforms check for bounding box.

A separate issue observed while working on this (which may or may not be an issue) is that currently crpix is considered 0-indexed while in FITS WCS standard it is 1-indexed.

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.89%. Comparing base (728b877) to head (9ff6bc2).
Report is 3 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff             @@
##             main     #326       +/-   ##
===========================================
+ Coverage   29.56%   86.89%   +57.33%     
===========================================
  Files          37       49       +12     
  Lines        8332     8946      +614     
===========================================
+ Hits         2463     7774     +5311     
+ Misses       5869     1172     -4697     

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

[kmacdonald-stsci]

The new return tuple from _calculate_offsets needs to be verified the docstring and computation are correct. Other than that, it looks good.

[kmacdonald-stsci]

In the docstring for _calculate_offsets it says shifts are to be subtracted, but they are added here. Is the doctrstring wrong or is the calculation wrong?

[mcara]

Neither is wrong: basically shifts are equivalent to 0-indexed CRPIX and should be subtracted from input coordinates to the WCS transformation. However, here we are adjusting the bounding box coordinates (not input coordinates to the WCS transform) so that it "moves" with the CRPIX value. Currently bounding box stays glued to the (0, 0) corner regardless of the output image shape or CRPIX location.

[emolter]

Looks good but just a few small questions

[emolter]

Does this mean that these offsets are always equal to crpix? If so, what is the point of returning this, if crpix is an input to the function? If not, can you clarify this note a little bit?

[emolter]

Why was this change necessary? Does this shift actually subtract 1 as the comment indicates?

[mcara]

Oh, this was a leftover from testing

[mcara]

The comment is incorrect either way. The default value for the shift is 0 anyway. So my change does not have any effect. I'll undo it.

[mcara]

The bigger issue is: should the bounding box of the resampled image follow (i.e., be centered) at CRPIX or should it always be fixed to the bottom-left corner of the resampled image.

The difference between the two approaches:

1. bbox follows crpix: If user sets a large output shape for the resampled image and places rcpix at the middle then most (valid) data will be centered at the center of the output. User can control where data will be by modifying crpix.
2. bbox is fixed: Then no matter what output shape user chooses, the input data will appear only in the bbox which is situated in the lower-left corner of the resampled image (when with_bounding_box for inverse WCS is enabled). By changing CRPIX, user shifts where data fall in the output but the bbox is not shifted and so it acts as a mask. So, as an example, if we increase CRPIX, we will move data to the right and there will be an empty region (no data) near the origin (BLC) but the data will not be "full" as bbox will cut off data on the right/top side. That is, we will be moving data out of the "view" (which, in this case, is the bbox and it is stationary) and the data will be trimmed by the bbox.

I think currently 2nd approach is implemented (albeit with a bug). 1st approach seems to be more intuitive as it allows control of where data will be in the output image and it is intuitive to estimate this based on the output shape. In the 2nd approach output shape larger than bbox makes no sense at all as it will be filled with 0s or NaN.

I know this is kind of a philosophical question but it is important to make a decision and based on this I will know how to fix spacetelescope/jwst#9017.

CC: @nden @emolter @melanieclarke

[mcara]

I have addressed most (all) comments, I believe. Along the way, fixed a typo in ramp test.

[mcara]

Just to be clear, this PR takes approach #1 in my previous (-2) comment. I think it is more intuitive (to the user) and kind of makes sense: bbox is moving with CRPIX thus allowing user to control where in the image the data is resampled.

With the other approach, both output_shape and crpix are pretty much useless since resampled pixels are contained only within the bounding box that is forced to stay in the bottom-left corner of the image. Modifying crpix in this case only can move valid pixels out of the bounding box. One problem is that we do not know (in the resample step) what this bounding box is and thus we don't know where to choose CRPIX. With a poor initial choice, pixmap is all nan and resample crashes before we can see what was the computed bounding box. It's counterintuitive.

[mcara]

Oh, there is a third idea:

3. Make the bounding box using the entire array shape. First, drizzle figures out overlaps as it resamples. Bounding boxes on the output images do not help much, unless that is what the user wants. Second, output WCS usually is a distortion-free WCS that is easily invertible so guarding against out-of-domain values is not necessary here. So, users can provide a bounding box with their custom WCS if that is what they want but in an automated pipeline, why impose one at all?

This actually makes me think that approach #1 is probably the better one if we are going to compute a restrictive bounding box in the pipeline. Or option #3.