Generalization classes for FITS cutouts #136
Conversation
There was a problem hiding this comment.
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.
| 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]] |
There was a problem hiding this comment.
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?
| 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, |
There was a problem hiding this comment.
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.
| 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) |
There was a problem hiding this comment.
Could this be simplified to return create_test_imgs(request.param, 50, 6, dir_name=tmpdir)?
| 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] | ||
|
|
There was a problem hiding this comment.
same with this, create_test_imgs(request.param, ..).
| assert new_dir in cutout_files[0] | ||
| assert path.exists(new_dir) # new directory should now exist | ||
|
|
||
| cutout_hdulist.close() |
There was a problem hiding this comment.
Since cutout_hdulist is being iterated over, should this be within the loop to ensure each instance gets closed properly?
| # Cutout each input file | ||
| for file in self._input_files: | ||
| self._cutout_file(file) | ||
|
|
There was a problem hiding this comment.
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?
This PR implements the generalized architecture for creating cutouts from FITS files. It implements
Cutout,ImageCutout, andFITSCutout. It also changes the functions incutouts.pyto 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:
FITSCutoutwill eventually use the_get_cutout_datamethod defined inImageCutoutthat usesastropy.nddata.Cutout2D. Unfortunately, there is an issue with using thesectionattribute of anImageHDUobject that I've put in a PR to Astropy to fix. We could choose to usedatainstead ofsection, 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.test_cutouts.py. The main difference is thattest_FITSCutout.pyuses theFITSCutoutclass directly whiletest_cutouts.pyuses thefits_cutandimg_cutfunctions. I've essentially copy-pasted the old code and added some assertions and cases to increase coverage.