Generalization classes for FITS cutouts #136

snbianco · 2025-01-23T22:44:22Z

This PR implements the generalized architecture for creating cutouts from FITS files. It implements Cutout, ImageCutout, and FITSCutout. It also changes the functions in cutouts.py to call the aforementioned classes and updates the test suite with a new file, test_FITSCutout.py.

Here is a class diagram of the new generalized architecture, though it may not be fully updated: https://innerspace.stsci.edu/display/MASTC/Class+Diagram

Considerations:

While implementing this new generalized architecture, I'm focusing on restructuring the code rather than adding features or making significant changes. The existing API should have no breaking changes, for now.
Ideally, FITSCutout will eventually use the _get_cutout_data method defined in ImageCutout that uses astropy.nddata.Cutout2D. Unfortunately, there is an issue with using the section attribute of an ImageHDU object that I've put in a PR to Astropy to fix. We could choose to use data instead of section, but this significantly worsens performance. I'm not sure when Astropy's next release will be, so for now, I think we will have to leave in the code that cuts out the data array and modifies the WCS for the cutout.
The new test class looks VERY similar to test_cutouts.py. The main difference is that test_FITSCutout.py uses the FITSCutout class directly while test_cutouts.py uses the fits_cut and img_cut functions. I've essentially copy-pasted the old code and added some assertions and cases to increase coverage.

AlexReedy

This is looking good to me!

havok2063

This overall looks like a good start to me. I think one thing to consider during this refactor is where spectral cutouts might fit in here. Slicing a spectrum in wavelength is fairly trivial but perhaps astrocut could provide some helper functions for efficient bulk slicing.

Or if we enable cutouts for IFU cubes, for JWST and SDSS MaNGA, we might want the option to cut both in spatial and spectral directions via this tool.

havok2063 · 2025-01-31T18:35:37Z

astrocut/FITSCutout.py

+    def _get_cutout_wcs(self, img_wcs: WCS, cutout_lims: np.ndarray) -> WCS:
+        """
+        Starting with the full image WCS and adjusting it for the cutout WCS.
+        Adjusts CRPIX values and adds physical WCS keywords.
+
+        Parameters
+        ----------
+        img_wcs : `~astropy.wcs.WCS`
+            WCS for the image the cutout is being cut from.
+        cutout_lims : `numpy.ndarray`
+            The cutout pixel limits in an array of the form [[ymin,ymax],[xmin,xmax]]


Some FITS products use gwcs, e.g some JWST miri or nirspec cubes. They actually store it within an asdf extension. And since ASDF files are basically dictionary trees, we might eventually have asdf files that store a FITS wcs as a dictionary.

In case we want to add support for JWST cube cutouts in the future, I wonder if we want to abstract out the WCS logic into their own classes, for fits-wcs/gwcs, that could be used by either FitsCutout or ASDFCutout? Not necessarily something to do for this PR, but maybe a follow-up?

This is a great point; I didn't realize that some FITS products are already using GWCS. Since the WCS logic will need to be used by both FITSCutout and ASDFCutout, I think the best place to put it would be the ImageCutout class. That way, both FITSCutout and ASDFCutout have access to the logic as children of ImageCutout. I will have to think some more on the best way to parse the type of WCS that a specific file has. Since this epic focuses on porting over the existing logic, I'll leave this for a follow-up PR.

havok2063 · 2025-01-31T18:54:28Z

astrocut/cutouts.py

+def img_cut(input_files: List[Union[str, Path, S3Path]], coordinates: Union[SkyCoord, str], 
+            cutout_size: Union[int, np.ndarray, Quantity, List[int], Tuple[int]] = 25, stretch: str = 'asinh', 
+            minmax_percent: Optional[List[int]] = None, minmax_value: Optional[List[int]] = None, 


Can you add a "Returns" entry in the docstring? The name img_cut seems a bit confusing to me. Its primary difference from fits_cut is to make color images from FITS? img_cut sounds like I'm making cutouts from non-fits images, like jpeg.

With the abstraction into FITSCutout are these two functions still needed? The primary user endpoint seems to be FITSCutout and both just call that class with different options.

Yep, I definitely see what you're saying about the function name (I don't love it either). My motivation for leaving these functions as they are is so that Astrocut stays backwards compatible.

That makes sense! Can we add a comment, or sentence in the docstring, then clarifying we're keeping these for backwards compatibility?

havok2063 · 2025-01-31T18:56:31Z

astrocut/tests/test_FITSCutout.py

+    if request.param == 'SPOC':
+        return create_test_imgs('SPOC', 50, 6, dir_name=tmpdir)
+    else:
+        return create_test_imgs('TICA', 50, 6, dir_name=tmpdir)


Could this be simplified to return create_test_imgs(request.param, 50, 6, dir_name=tmpdir)?

havok2063 · 2025-01-31T18:57:20Z

astrocut/tests/test_FITSCutout.py

+    if request.param == 'SPOC':
+        return create_test_imgs('SPOC', 50, 1, dir_name=tmpdir,
+                                basename="img_badsip_{:04d}.fits", bad_sip_keywords=True)[0]
+    else:
+        return create_test_imgs('TICA', 50, 1, dir_name=tmpdir,
+                                basename="img_badsip_{:04d}.fits", bad_sip_keywords=True)[0]
+


same with this, create_test_imgs(request.param, ..).

havok2063 · 2025-01-31T19:00:22Z

astrocut/tests/test_FITSCutout.py

+    assert new_dir in cutout_files[0]
+    assert path.exists(new_dir)  # new directory should now exist
+
+    cutout_hdulist.close()


Since cutout_hdulist is being iterated over, should this be within the loop to ensure each instance gets closed properly?

havok2063 · 2025-01-31T19:06:54Z

astrocut/ImageCutout.py

+        # Cutout each input file
+        for file in self._input_files:
+            self._cutout_file(file)
+


Did astrocut ever use multiprocessing for handling batch cutouts? Given some of the roman reqs and the tests you did before on mass cutouts, we might want to create optional modes of batch processing files. Not something for this PR, but maybe something for later?

We use ThreadPoolExecutor in cube_cut, but definitely agree that this is something we should add to other classes at some point.

snbianco · 2025-02-11T01:02:39Z

Re: spectral cutouts - this is a great point and would make an excellent new feature for Astrocut. As far as where this would fit in the new architecture, I envision it as a new class inheriting directly from Cutout, so that it would be on the same level as ImageCutout and CubeCutout.

snbianco · 2025-02-17T17:20:32Z

Is this okay to merge? The next MR will be ASDF cutouts, so changes can also be done there.

havok2063 · 2025-02-17T18:24:35Z

I'm good with merging.

snbianco added 6 commits January 23, 2025 17:43

Generalization classes for FITS cutouts

bb65ecd

Start on tests

797287b

Better handle ASDF output

379d251

Test fix

d455f4e

Tests, style, more checks

4f4b8cf

test case for input file with no image data

d1b7e9a

snbianco marked this pull request as ready for review January 28, 2025 01:45

snbianco requested review from AlexReedy, falkben and havok2063 January 28, 2025 13:55

AlexReedy reviewed Jan 28, 2025

View reviewed changes

havok2063 reviewed Jan 31, 2025

View reviewed changes

snbianco added 8 commits February 10, 2025 20:03

Address PR comments

fae7c09

Fix style and bug

0cf2a0c

Note about backwards compatibility

bbb706b

Slight restructuring

be7be72

small typing changes

af0e7fa

Move normalize_img tests into different file

6ed228d

Delete ImageCutout tests from test_FITSCutout.py

4aac041

style fixes

021335b

havok2063 self-requested a review February 17, 2025 18:23

havok2063 approved these changes Feb 17, 2025

View reviewed changes

snbianco merged commit 6c2bca0 into main Feb 17, 2025
8 checks passed

snbianco deleted the ASB-30252-FITS-Cutout branch February 17, 2025 19:00

Generalization classes for FITS cutouts #136

Generalization classes for FITS cutouts #136

Uh oh!

Conversation

snbianco commented Jan 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlexReedy left a comment

Choose a reason for hiding this comment

Uh oh!

havok2063 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snbianco Feb 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snbianco Feb 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snbianco commented Feb 11, 2025

Uh oh!

snbianco commented Feb 17, 2025

Uh oh!

havok2063 commented Feb 17, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

snbianco commented Jan 23, 2025 •

edited

Loading

snbianco Feb 11, 2025 •

edited

Loading

snbianco Feb 11, 2025 •

edited

Loading