Simplify ResampleStep output_wcs interface

[jdavies-st]

This PR simplifies the interface to match a resampled output grid to an existing image in ResampleStep. Currently, one needs to do something like the following to resample data onto the same output grid as an existing reference mosaic:

import asdf
from jwst.steps import ResampleStep


with asdf.open("reference_mosaic_i2d.fits") as af:
    wcs = af.tree["meta"]["wcs"]
    new_af = asdf.AsdfFile(tree=dict(wcs=wcs))
    new_af.write_to("reference_mosaic_wcs.asdf")
    ysize, xsize = af.tree["data"].shape

result = ResampleStep.call("my_asn.json", output_wcs="reference_mosaic_wcs.asdf", output_shape=(xsize, ysize))

This PR improves the interface by not needing one to extract the wcs from the reference mosaic or figure out its data shape. Instead, just pass the reference mosaic itself as the output wcs:

from jwst.steps import ResampleStep


result = ResampleStep.call("my_asn.json", output_wcs="reference_mosaic_i2d.fits")

I have also updated the ResampleStep.spec comments as these are not comments for use by developers, but are the documentation to the strun command line interface.

Checklist for maintainers

• 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
• Make sure the JIRA ticket is resolved properly

[braingram]

Thanks for providing the example in the description of this PR. It has brought to my attention the usage of asdf.open to open a fits file. This is deprecated in asdf and will be removed in asdf 3.0 (which will be released soon). Please see these docs for more information:
https://asdf.readthedocs.io/en/latest/asdf/deprecations.html#asdf-in-fits-deprecation

It appears that to retain the functionality in your example the resample step will need to be updated to use stdatamodels to open the file (which now supports AsdfInFits).

I suspect this was not caught until now as the existing test uses an asdf file (instead of an AsdfInFits file) to supply the wcs. The changes in this PR to handle ImageModel and SlitModel instances may also require changes to accommodate the use of stdatamodels (instead of asdf) to open fits files (like in the test modification included in this PR).

[jdavies-st]

Ah, thanks @braingram. I will open with datamodels.open() then. That will actually simplify and also allow one to pass the datamodel in memory.

Any idea what sort of exception will be raised in the future in asdf if I try to open a FITS file with asdf.open()?

[braingram]

Using datamodels.open sounds like a great idea!

The error raised when a fits file is passed to asdf.open can be seen in the devdeps testing run (asdf main has already dropped AsdfInFits support):
https://github.com/spacetelescope/jwst/actions/runs/6301749854/job/17107497466?pr=7971#step:10:710
The error from the log is:

E           ValueError: Does not appear to be a ASDF file.

[jdavies-st]

The changes in this PR to handle ImageModel and SlitModel instances may also require changes to accommodate the use of stdatamodels (instead of asdf) to open fits files (like in the test modification included in this PR).

Now fixed in 2e8afbd

Oh, and I see the devdeps job caught this. And its a ValueError. 🚀

[codecov]

Codecov Report

All modified lines are covered by tests ✅

Files	Coverage Δ
jwst/resample/resample_step.py	89.69% <100.00%> (+0.32%)	⬆️

📢 Thoughts on this report? Let us know!.

[braingram]

Thanks! Regression tests run with no errors:
https://plwishmaster.stsci.edu:8081/blue/organizations/jenkins/RT%2FJWST-Developers-Pull-Requests/detail/JWST-Developers-Pull-Requests/971/pipeline/201

[hbushouse]

@mcara @nden Would really like to have your opinions on this change.

[mcara]

Actually, originally when I implemented this parameter, output_wcs, I was thinking of allowing this usage. But I do not remember why I decided not to do this. I think this looks good.

One thing that worries me is that it allows users to easily provide any data model as output_wcs, even a cal file. With ASDF one has to make a conscious effort to create that file (hopefully from a resampled image). For the very least we should add additional note to the docs that this WCS is expected to be without distortions and that there is no check for this.

[mcara]

Can this be a SlitModel in a ResampleStep?

[jdavies-st]

Theoretically yes, but practically I think it will always be ImageModel.

[nden]

This function is also called by ResampleSpecStep, right?
Does passing an external WCS to spectral observations work?

[mcara]

This function is also called by ResampleSpecStep, right? Does passing an external WCS to spectral observations work?

It should.

[mcara]

Thanks! Regression tests run with no errors:
https://plwishmaster.stsci.edu:8081/blue/organizations/jenkins/RT%2FJWST-Developers-Pull-Requests/detail/JWST-Developers-Pull-Requests/971/pipeline/201

I do not think there is a test that actually uses an external reference WCS.

[jdavies-st]

I do not think there is a test that actually uses an external reference WCS.

Right, just the unit test does, which I updated (perhaps not optimally).

[nden]

Would it be too much burden to require the wcs attribute in an asdf file to be under meta as well, to make it uniform? An argument for this is that this pipeline is supposed to work equivalently with asdf files, so it's conceivable some time in the future the reference file will be an i2d.asdf file with the wcs under model.meta.wcs.
I think the uniformity of the file structure may help users, though if it's considered too much of a burden, I'd be OK with the current proposal.

[jdavies-st]

I think that would be an API change, as up to this point it's been required to be at the top level. I.e. this PR doesn't change that. I do know there's existing analysis code in the wild (ERO CEERS code on github) that specifically uses this functionality.

Agree it would be good to have consistency. Maybe better to move wcs out of meta instead, something we've talked about off and on before.

[nden]

This function is also called by ResampleSpecStep, right?
Does passing an external WCS to spectral observations work?

[nden]

If I'm reading this correctly, it means that if an external reference file is provided the data shape takes precedence to the bounding box of the WCS (assuming it exists too). Is this the correct decision? It seems to me the bounding_box should take precedence.

[nden]

Not part of this PR but the line below (line 189) seems wrong, especially when wcs is None

[mcara]

Actually, originally when I implemented this parameter, output_wcs, I was thinking of allowing this usage. But I do not remember why I decided not to do this. I think this looks good.

One thing that worries me is that it allows users to easily provide any data model as output_wcs, even a cal file. With ASDF one has to make a conscious effort to create that file (hopefully from a resampled image). For the very least we should add additional note to the docs that this WCS is expected to be without distortions and that there is no check for this.

I think another reason for not allowing FITS as reference files is that users may try to use the same reference files from drizzlepac (used to process HST data) to resample JWST to the same grid as some of their HST observations.

I think this PR has a potential to open a lot of issues later unless everything is well documented and there are checks in the code to provide meaningful error messages for unsupported situations.

Saving a WCS to a file is not very difficult:

from jwst.datamodels import ImageModel
import asdf
im = ImageModel('resampled_i2d.fits')
af = asdf.AsdfFile({'wcs': im.meta.wcs})
af.write_to('ref_wcs.asdf')
# or
# asdf.AsdfFile({'wcs': ImageModel('resampled_i2d.fits').meta.wcs}).write_to('ref_wcs.asdf')