astropy / specutils

An Astropy coordinated package for astronomical spectroscopy. Maintainers: @nmearl @rosteen @keflavich @eteq
http://specutils.readthedocs.io/en/latest/
161 stars 124 forks source link

add: new SDSS V datatype loaders #1107

Closed rileythai closed 4 months ago

rileythai commented 7 months ago

Within the 5th generation of the SDSS, there are several new data products, and many of the access methods for existing datatypes have been changed.

This pull request will add new default loaders for the new/updated SDSS-V spectra data products in a new file called sdss_v.py, along with unit tests in test_sdss_v.py, which will allow for automatic loading of SDSS-V data into Spectrum1D/List objects, and directly into jdaviz.

For mwm and spec files, the first HDU with data is loaded with the Spectrum1D loader, whereas all spectra within a file are loaded with the SpectrumList loader.

Remaining questions:

rileythai commented 7 months ago

This is a great start. One thing we might want to have a think on, or get some feedback from the specutils folks, is for the multi loaders, if we should use SpectrumList or Spectrum1D. If all the spectra in that file are on the same wavelength solution (and shape), we may want to use Spectrum1D as the container to take advantage of its numpy array operations. If they really are different, then we can stick with SpectrumList, which is just a regular python list.

The main issue is jdaviz. It won't load Spectrum1D objects with 2D flux arrays (not implemented yet). As you said -- if someone who knows a little more amount specutils + jdaviz could comment on this, it'd be great.

havok2063 commented 7 months ago

The main issue is jdaviz. It won't load Spectrum1D objects with 2D flux arrays (not implemented yet). As you said -- if someone who knows a little more amount specutils + jdaviz could comment on this, it'd be great.

If it's primarily a jdaviz issue, then it should be fixed there. We can open a new issue in jdaviz if there isn't one already. specutils has more utility outside of Jdaviz, so we want to make sure we're doing the "right" thing here. I'm not suggesting we change anything, just saying.

havok2063 commented 6 months ago

@rileythai I think there's something wrong with the SDSS-V identify functions on the loaders. The following code should correctly identify the format of this file either as SDSS-V spec multi or SDSS-V spec.

import specutils
from specutils.io.registers import identify_spectrum_format

file = '/Users/Brian/Work/sdss/sas/sdsswork/bhm/boss/spectro/redux/v6_0_6/spectra/lite/017057/59631/spec-017057-59631-27021598108289694.fits'

identify_spectrum_format(file, specutils.SpectrumList)
[]

This function basically loops over all the identifiers in the formats table, io_registry.get_formats() for the input object type, and returns the format of the best match. If this can return successfully, then Spectrum1D.read(file) will work without manually specifying the format.

Examples for MaNGA, and spec-lite for SDSS-IV.

# manga file
identify_spectrum_format("redux/v3_1_1/8485/stack/manga-8485-1901-LOGCUBE.fits.gz", specutils.Spectrum1D)
'MaNGA cube'

# eboss file
identify_spectrum_format("eboss/spectro/redux/v5_10_0/spectra/lite/3606/spec-3606-55182-0537.fits", specutils.SpectrumList)
'SDSS-III/IV spec'
rileythai commented 6 months ago

@rileythai I think there's something wrong with the SDSS-V identify functions on the loaders. The following code should correctly identify the format of this file either as SDSS-V spec multi or SDSS-V spec.

I had it check for an OBSERVAT column in the primary HDU, which doesn't exist in the file you've used. I've removed it since it probably doesn't appear in other files (99eccef), so it should work now for other similar files.

In [1]: import specutils

In [2]: from specutils.io.registers import identify_spectrum_format

In [3]: path = "/home/riley/uni/rproj/data/"

In [4]: identify_spectrum_format(path + "spec-017057-59631-27021598108289694.fits", specutils.SpectrumList)
Out[4]: ['SDSS-V spec', 'SDSS-V spec multi']
rosteen commented 4 months ago

Thanks for this contribution, I'm trying to make some time to review in the next week. In the meantime, would you resolve the conflict with main? Cheers.

codecov[bot] commented 4 months ago

Codecov Report

Attention: 32 lines in your changes are missing coverage. Please review.

Comparison is base (c646007) 71.11% compared to head (1fdb4a2) 72.21%.

Files Patch % Lines
specutils/io/default_loaders/sdss.py 21.21% 26 Missing :warning:
specutils/io/default_loaders/sdss_v.py 96.64% 6 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1107 +/- ## ========================================== + Coverage 71.11% 72.21% +1.10% ========================================== Files 61 62 +1 Lines 4248 4427 +179 ========================================== + Hits 3021 3197 +176 - Misses 1227 1230 +3 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.