AL-852: GWCS inverse transform should respect its bounding box #8554
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
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
☔ View full report in Codecov by Sentry. |
|
The failing oldestdeps tests appear to show that asdf needs a minimum of at least 3.3 now since that's what GWCS needs |
fixed outlier test failures
There was a problem hiding this comment.
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.
|
Regression test run: https://github.com/spacetelescope/RegressionTests/actions/runs/12395581275/job/34601785739 MIRI LRS differences are expected, since the bounding_box has changed. |
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 |
There was a problem hiding this comment.
These changes look good for me other than the comments made by @emolter which look relatively trivial.
Does this warrant a change log fragment?
|
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. |
… Bump min version of asdf. Fix tweakreg step test.
| forward_transform = wcs1.pixel_to_world_values | ||
| backward_transform = wcs2.world_to_pixel_values | ||
| wcs_no_bbox = deepcopy(wcs2) |
There was a problem hiding this comment.
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?
| @pytest.mark.parametrize('shape', [(1205, 1100)]) | ||
| @pytest.mark.parametrize('crpix', [(100, 101), (101, 101)]) | ||
| @pytest.mark.parametrize('crval', [(22.01, 12), (22.15, 12.01)]) | ||
| @pytest.mark.parametrize('shape', [(10205, 10100)]) |
There was a problem hiding this comment.
This is way, way too large for a test. I think it is an indication that crval is wrong if you need to make output shape this large in order to get data within the output.
There was a problem hiding this comment.
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.
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)
CHANGES.rstwithin the relevant release sectionHow to run regression tests on a PR