Use different coords for 3d and 1d Spectrum1D objects

[javerbukh]

Pull Request Template

Description

Fixes #28 Uses different coords depending on if the object is a Spectrum1D of shape 1 or other (in the jdaviz case, 3). The motivation for this change is to implement the latest changes to Spectrum1D that replace the SpectralCube functionality with NDCube.

[codecov[bot]]

Codecov Report

Merging #29 (3a72b76) into master (930fd84) will decrease coverage by 0.86%. The diff coverage is 75.00%.

@@            Coverage Diff             @@
##           master      #29      +/-   ##
==========================================
- Coverage   97.27%   96.41%   -0.87%     
==========================================
  Files          13       15       +2     
  Lines         991     1003      +12     
==========================================
+ Hits          964      967       +3     
- Misses         27       36       +9     

Impacted Files	Coverage Δ
glue_astronomy/translators/spectrum1d.py	95.94% <75.00%> (-1.28%)	:arrow_down:
...onomy/io/spectral_cube/tests/test_spectral_cube.py	85.71% <0.00%> (-14.29%)	:arrow_down:
glue_astronomy/__init__.py	71.42% <0.00%> (ø)
glue_astronomy/conftest.py	100.00% <0.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 930fd84...3a72b76. Read the comment docs.

[pllim]

(Drive-by review.)

Failures probably unrelated but I wonder if this change needs a new test, but adding a new test is meaningless if CI is failing anyway. 🤷

Not familiar with the internals but will this assumption between shape and WCS always hold?

[rosteen]

I haven't tested this yet, but first thought: is the check for 1D really needed, or can we just use obj.wcs for all cases? A Spectrum1D created with a spectral_axis (rather than a WCS) still creates an appropriate WCS on the backend that should work here (I hope).

[javerbukh]

It does not work.

Setting data = Data(coords=obj.wcs) gives the following traceback in the SpecvizExample notebook:

~/Documents/jdaviz_dev/glue-astronomy/glue_astronomy/translators/spectrum1d.py in to_object(self, data_or_subset, attribute, statistic)
     81         else:
     82 
---> 83             raise TypeError('data.coords should be an instance of WCS '
     84                             'or SpectralCoordinates')
     85 

TypeError: data.coords should be an instance of WCS or SpectralCoordinates

And instead setting the following two lines

coords = SpectralCoordinates(obj.wcs)
data = Data(coords=coords)

gives the traceback:

~/Documents/jdaviz_dev/glue-astronomy/glue_astronomy/translators/spectrum1d.py in to_data(self, obj)
     23     def to_data(self, obj):
     24         # if len(obj.shape) == 1:
---> 25         coords = SpectralCoordinates(obj.wcs)
     26         #     data = Data(coords=coords)
     27         # else:

~/Documents/jdaviz_dev/glue-astronomy/glue_astronomy/spectral_coordinates.py in __init__(self, values)
     13 
     14     def __init__(self, values):
---> 15         self._index = np.arange(len(values))
     16         self._values = values
     17         super().__init__(n_dim=1)

TypeError: object of type 'SpectralGWCS' has no len()

[rosteen]

Thought about this a bit more, the problem you're seeing is that Spectrum1D creates a GWCS subclass for the WCS when there is a spectral_axis but no WCS provided on initialization. This conflicts with the fact that the glue-astronomy translator only checks to see if the coords is a WCS or spectral_axis (no case for GWCS). So I think the real play here is to always use the WCS in the Spectrum1D translator's to_data method, and add the handling for the GWCS to the to_object method.

The other complication is that the simple dimensionality check doesn't work (I think) because you can create a Spectrum1D with multidimensional flux where you only specify the spectral_axis, in which case you also end up with the GWCS.

[astrofrog]

Superseded by #36