New Feature: ASDF Cutout Functionality

[jaymedina]

Background

The Roman Space Telescope will have its data stored in a new file format called ASDF. In order to continue supporting our userbase and enabling productive science, the astrocut tool needs to be expanded to allow for the creation of cutouts of Roman mission products.

Changes

• Enable ASDF file format cutout functionality in astrocut with a proof-of-concept script (asdf_cutouts.py)
• Generalize the asdf_cutouts.py enough such that it can "merged" into the cutouts.py script
• Unit tests (DON'T UPLOAD ROMAN DATA TO REPO)
• Documentation update
• Dependency updates

Preview

[hmlewis-astro]

Thanks, Jenny! This looks like it's in good shape for us to pick-up!

[havok2063]

This looks good to me as well.

[havok2063]

Shall we mark this PR as ready to try and get this merged in?

[braingram]

Thanks for making these changes. It's great to see ASDF support!

I left a few comments mainly related to using with to help closing opened files. Please feel free to reach out with any asdf questions.

Are there plans to add asdf file support to the test suite? If so feel free to ping me on the PR and I can start looking at adding astrocut tests to the asdf downstream CI (to make sure that changes in asdf do not negatively impact astrocut).

Please feel free to reach out with any asdf questions.

[havok2063]

@braingram I resolved your comments. I think we have eventual plans to add asdf support to the test suite. I'm not sure if those tests should be added now for this code, or if we should wait, since we are planning to fold these changes into the core astrocut code through some refactor and generalization. But if people want them in now, I can try to add some.

[havok2063]

@braingram so the minimum python that astrocut supports is 3.8. It looks like roman-datamodels > 0.17 only supports 3.9, while rad > 0.17 still supports 3.8. roman-datamodels > 14 does support 3.8. I'm not familiar enough with either product. Given this, would you recommend adding back in the rad package dependency or lowering the roman-datamodels min version to >14?

[falkben]

@braingram so the minimum python that astrocut supports is 3.8. It looks like roman-datamodels > 0.17 only supports 3.9, while rad > 0.17 still supports 3.8. roman-datamodels > 14 does support 3.8. I'm not familiar enough with either product. Given this, would you recommend adding back in the rad package dependency or lowering the roman-datamodels min version to >14?

I think we should drop python 3.8. Newer releases of astropy also dropped support of 3.8.

[falkben]

@braingram I resolved your comments. I think we have eventual plans to add asdf support to the test suite. I'm not sure if those tests should be added now for this code, or if we should wait, since we are planning to fold these changes into the core astrocut code through some refactor and generalization. But if people want them in now, I can try to add some.

I think it would be good to add tests here, where we add the functionality.

[havok2063]

I think we should drop python 3.8. Newer releases of astropy also dropped support of 3.8.

Should that be a separate PR? Is there more to it than bumping the version and removing 3.8 from the CI workflow? Do we need to coordinate that bump with other related software that use astrocut as a dependency?

[braingram]

I'll try to provide some context here but I'm not sure I can recommend a solution.

Both rad and roman_datamodels haven't reached a 'stable' state yet. It's not uncommon to see a breaking change be added that makes opening older files impossible (and similarly opening new files in older versions impossible). Without testing I would not bet on 0.14 opening any current file.

Perhaps rad and roman_datamodels could be included as optional requirements? This would allow the main install/CI to retain compatibility and the 'roman' options only tested in a specific job using a newer version of python.

[falkben]

Should that be a separate PR?

I'm fine with it either way. Since it'll be required to go in before this can pass CI, it could be in this or a separate PR that goes in first. If you like I could work on it, but no promises I can get it done quickly.

Is there more to it than bumping the version and removing 3.8 for the CI workflow?

Bumping the version throughout the project, making sure the pypi classifiers are correct. i think it's relatively trivial. We should start testing on 3.12 soon. that could also be done at the same time.

Do we need to coordinate that bump with other related software that use astrocut as a dependency?

The only thing I can think of are tesscut. lightkurve calls tesscut over HTTP, so it's not impacted. tesscut is on 3.11 now, so it's not a big deal to drop 3.8. Github doesn't recognize any dependents here: https://github.com/spacetelescope/astrocut/network/dependents anything still on 3.8 can also just install an older version of astrocut. we'll probably not push to pypi immediately, either. or are you thinking of cutting a release soon?

[scfleming]

Regarding adding tests, I just want to remind everyone we do not want to release any test Roman data that can be publicly accessed, since there is no official Roman Level 2 test datasets released at MAST yet. Ideally any test would use a purely synthetic asdf file, or can retrieve the file it needs to operate on from a protected source. Or the test itself doesn't go public yet.

[havok2063]

Perhaps rad and roman_datamodels could be included as optional requirements? This would allow the main install/CI to retain compatibility and the 'roman' options only tested in a specific job using a newer version of python.

I like the idea of making roman-datamodels an optional dependency for now. Once we migrate this code into the core astrocut code and/or roman data becomes available, we can then make it a required dep. Since we're dropping 3.8 support, we can consolidate it down to just roman-datamodel > 0.17.

we'll probably not push to pypi immediately, either. or are you thinking of cutting a release soon?

Saw the 3.8 PR and approved. I don't think we are planning on releasing soon.

Regarding adding tests, I just want to remind everyone we do not want to release any test Roman data that can be publicly accessed, since there is no official Roman Level 2 test datasets released at MAST yet. Ideally any test would use a purely synthetic asdf file, or can retrieve the file it needs to operate on from a protected source. Or the test itself doesn't go public yet.

Yeah I would opt for creating a completely fake dataset for the test suite. We can easily update it as the datamodel changes. Then once roman data are available and maybe even public, then we add tests with real data.

[braingram]

For test data I opened an issue with roman_datamodels to expose something to the public api. There are several 'maker_utils' for generating test data. Here's an example use in jdaviz:
https://github.com/spacetelescope/jdaviz/blob/e8f125ebe375f938ed3a7b2af97e1a1c3dd5b531/jdaviz/configs/imviz/tests/utils.py#L241
It seems more "future-proof" to rely on something added to roman_datamodels as I'm not sure after looking at the jdaviz example if the WCS is kept up-to-date with changes in roman_datamodels.

[codecov]

Codecov Report

Attention: Patch coverage is 96.15385% with 1 line in your changes missing coverage. Please review.

Project coverage is 94.56%. Comparing base (3be3957) to head (e182dd4).
Report is 43 commits behind head on main.

Files with missing lines	Patch %	Lines
astrocut/asdf_cutouts.py	96.15%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #105      +/-   ##
==========================================
+ Coverage   94.53%   94.56%   +0.02%     
==========================================
  Files           8        9       +1     
  Lines        1446     1472      +26     
==========================================
+ Hits         1367     1392      +25     
- Misses         79       80       +1     

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

[havok2063]

@falkben @braingram For now I've added some tests with fake data to make sure the basic functionality is there. Let me know if this seems reasonable to y'all.

[falkben]

thanks very much for adding the tests, they look good. just a few minor questions about how we are handling the file/file handle.

[braingram]

@havok2063 Thanks for making these improvements. Overall it looks good to me. Hopefully spacetelescope/roman_datamodels#299 will see some traction and there were be an easier way to make a test image (with wcs) in the future.

[havok2063]

@falkben I've consolidated the file IO into the main function, and added a small header keyword check. Included some extra cleanup, type annotations and docstrings, etc.