spacetelescope / jdaviz

JWST astronomical data analysis tools in the Jupyter platform
https://jdaviz.readthedocs.io/
BSD 3-Clause "New" or "Revised" License
142 stars 74 forks source link

Fix bug when loading level 2 nirspec MSA spectra into mosviz #1789

Open Jdaviz-Triage-Bot opened 2 years ago

Jdaviz-Triage-Bot commented 2 years ago

Reporter: Erik Tollerud

While trying to load a level 2 nirspec MSA file from MAST into mosviz, I got a curious error: mosviz.load_spectra(results['Local Path'][1], results['Local Path'][0]) IORegistryError: Format could not be identified based on the file name or contents, please provide a 'format' argument.

Tracing this a bit further, I tried loading the file as a Spectrum1D and got: specutils.Spectrum1D.read(results['Local Path'][1], format='JWST x1d')

which yielded a more useful error: RuntimeError: Input data has 71 spectra. Use SpectrumList.read() instead. which makes sense, so I then did specutils.SpectrumList.read(results['Local Path'][1], format='JWST x1d') which yielded the exact same error, which does not make sense!

I can provide the file to reproduce if needed, but would have to be via private message because it's currently proprietary data. (and probably any NIRSpec MSA level 2 files will do? So there might be something public that can be tested on.)


DISCLAIMER: This issue was autocreated by the Jdaviz Issue Creation Bot on behalf of the reporter. If any information is incorrect, please contact Duy Nguyen

eteq commented 2 years ago

I guess this may be really as a specutils issue, so might be just upstream fix required, but would be good to check that.

eteq commented 2 years ago

After the above didn't work I tried manually loading the 1D spectra like this:

x1df = fits.open(results['Local Path'][1])

specs1d = []
for hdu in x1df[1:-1]:
    qt = table.QTable.read(hdu)
    spec = specutils.Spectrum1D(spectral_axis=qt['WAVELENGTH'], 
                         flux=qt['FLUX'])
    specs1d.append(spec)

and then mv.load_data(specs1d) gave:

ValueError: No data found with the label 'MOS Table'

So then I also tried loading the 2d spectra like this:

s2ddm = datamodels.open(results['Local Path'][0])
specs2d = []
for s in s2ddm.slits:
    sc = specutils.SpectrumCollection(flux=s.data*u.Unit(s.meta.bunit_data), 
                                  wcs=s.meta.wcs)
    specs2d.append(sc)

and then mv.load_data(specs1d, specs2d) gave:

TypeError: metadata must be dictionary or FITS header
eteq commented 2 years ago

Note I can make a separate issue for https://github.com/spacetelescope/jdaviz/issues/1789#issuecomment-1295414989 if we think that's a separate and distinct issue. I would have expected a vector Spectrum1D to work really no matter where they came from.

pllim commented 2 years ago

Ah... maybe Mosviz is less flexible than Specviz2d and I was probably thinking about Specviz2d when I replied about the "yes you can load 1d separately" thingy...

pllim commented 2 years ago

metadata must be dictionary or FITS header

If metadata is not a dict or astropy.io.fits.Header, then what is it? Do you have a traceback for this one?

eteq commented 2 years ago

I tracked this down - it's because for some reason SpectrumCollection.meta defaults to None instead of an empty dict. So I manually set it, and then ran into another problem where the mosviz loader does something.meta = something_else, and SpectrumCollection doesn't allow setting of meta, which I fixed by just using.update instead. I can make a PR for just that, but it still doesn't solve the problem because then I get:

TypeError: Could not find a class to translate objects of type SpectrumCollection to Data

Which is pretty clear, but surprising to me: how do we even show 2D spectra that are not rectified without a SpectrumCollection adapter?

eteq commented 2 years ago

(This, though, might be a key difference between level 2 and 3 - I'm not sure, but I think maybe level 3 data is rectified and therefore can be Spectrum1D, where level 2 is not)

pllim commented 2 years ago

Could not find a class to translate objects of type SpectrumCollection to Data

But why would it have to (and no, I don't think we have that translator)? Did you get into the mos_spec1d_parser call? It would have tried to loop through the collection internally before passing in the individual spectra into glue. 🤔

https://github.com/spacetelescope/jdaviz/blob/c319c30c7744f59c62aefc59f2860ee6eea94a79/jdaviz/configs/mosviz/plugins/parsers.py#L213-L217

eteq commented 2 years ago

Oh I made the list of SpectrumCollections myself a la the snippet in https://github.com/spacetelescope/jdaviz/issues/1789#issuecomment-1295414989 because the built-in parser didn't work for level 2 files.

But I'm partly being a canary in a coal mine here, because there are lots of 2d spectral datasets that mosviz should be able to read that could be shoved into a SpectrumCollection. So I was expecting that to be acceptable as a 2D spectrum for mosviz because whether NIRSpec works or not, lots of other MOSs out there would look like this.

pllim commented 2 years ago

Ah, asking Mosviz to do this would be a new feature request and should be a separate ticket -- cc @camipacifici

from specutils import SpectrumCollection
from jdaviz import Mosviz

sc = SpectrumCollection(blah)

mosviz = Mosviz()
mosviz.load_data(sc)
eteq commented 2 years ago

Gotcha, agree, @pllim.

Note I did also just discover mv.load_1d_spectra(specs1d, None) does work if all I want are the 1D spectra, so that's a fine workaround for now. That doesn't address the original issue here that I can't load by file name even thought these are fairly standard JWST files from MAST, though, which may be a mosviz problem, may be specutils, or may be in between.

pllim commented 2 years ago

As for loading the particular file, AFAIK Jdaviz only ever promised to load level 3. I wasn't aware on any promises on level 2, especially if the WCS is only embedded in the ASDF layer and not in FITS header. Hard to tell what is wrong with that file. So pick a dev and debug in private... or if you want to have a look, the logic flow goes something like this when you call it:

https://github.com/spacetelescope/jdaviz/blob/c319c30c7744f59c62aefc59f2860ee6eea94a79/jdaviz/configs/mosviz/helper.py#L384

https://github.com/spacetelescope/jdaviz/blob/c319c30c7744f59c62aefc59f2860ee6eea94a79/jdaviz/app.py#L478

which then delegates the low-level logic to individual parser defined in

https://github.com/spacetelescope/jdaviz/blob/main/jdaviz/configs/mosviz/plugins/parsers.py

Good luck!

eteq commented 2 years ago

Hmm, I think we really should support level 2, but I will grant it may not make sense to do that until the more general problem of SpectralCollection is solved.

I'm surprised to hear "especially if the WCS is only embedded in the ASDF layer and not in FITS header", though - shouldn't we be using the gwcs over the FITS-WCS since the FITS-WCS is not the canonical WCS for anything in JWST? (This might be a speuctils issue not mosviz though! So if someone feels it's not mosviz's business we can move this to an issue there)

pllim commented 2 years ago

Re: FITS WCS vs GWCS

That was decided before I joined this project. If you look at parser codes throughout, most of them grabs FITS headers. I do not know why. Maybe GWCS was incomplete with simulated data? 🤷

For Imviz, it does prefer GWCS (which does not help you here at all) but in doing that, we run into the problem of GWCS refusing to extrapolate the sky coordinates when pixel is out of bounds, making inspecting stacked dithered images in Imviz a little painful because mouse position is solely based on the reference data.

pllim commented 2 years ago

But to make things more interesting, specutils does grab GWCS depending if your logic goes to the correct loader...

https://github.com/astropy/specutils/blob/695e475ca392eb1c5be8cfff30b144239b777136/specutils/io/default_loaders/jwst_reader.py#L422

pllim commented 2 years ago

But then in some cases, it grab wavelength directly from data table, so I dunno... depends on your data?

https://github.com/astropy/specutils/blob/695e475ca392eb1c5be8cfff30b144239b777136/specutils/io/default_loaders/jwst_reader.py#L322

camipacifici commented 2 years ago

Loading level 2 nirspec data is part of the grand scheme for mosviz, but we are not there yet.