Fix bug in translating 3D Spectrum1D with only spectral axis defined #61

rosteen · 2022-01-25T16:23:44Z

Description

If a 3D flux is input to a Spectrum1D along with a spectral_axis (rather than a full 3D WCS), the resulting object will have a spectral GWCS of only one dimension. Personally I think this behavior is not ideal and might need rethinking at some point, but in either case this fix will stop the translator from trying to swap axes on the WCS in this case, thus fixing (I hope) the bug @pllim ran into in spacetelescope/jdaviz#1040 (comment).

Fixes #60

…(rather than full WCS)

codecov · 2022-01-25T16:27:06Z

Codecov Report

Merging #61 (312a840) into main (7f3229d) will decrease coverage by 1.23%.
The diff coverage is 84.09%.

@@            Coverage Diff             @@
##             main      #61      +/-   ##
==========================================
- Coverage   95.80%   94.56%   -1.24%     
==========================================
  Files          15       15              
  Lines        1145     1123      -22     
==========================================
- Hits         1097     1062      -35     
- Misses         48       61      +13

Impacted Files	Coverage Δ
glue_astronomy/translators/spectrum1d.py	`89.91% <80.00%> (-2.66%)`	⬇️
...lue_astronomy/translators/tests/test_spectrum1d.py	`92.85% <92.85%> (-7.15%)`	⬇️

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 7f3229d...312a840. Read the comment docs.

rosteen · 2022-01-27T18:02:03Z

@astrofrog I was working on the route you suggested of using NDCube's CompoundLowLevelWCS for a general solution of padding the WCS needed for additional axes, but I'm unsure about how many of the properties you had coded into your 2D solution need to be kept in my version. Seems like we do need a high level WCS object here, but just using HighLevelWCSWrapper around the CompoundLowLevelWCS isn't sufficient. If you have any advice/comments based on where I'm at so far I'd appreciate it!

pllim · 2022-02-14T21:51:59Z

Okay. I got it. With this PR, it works if I pass in only the flux like this Spectrum1D(flux=np.random.random((10, 10, 5))), where 5 is the length of the spectral axis. If I pass in spectral_axis explicitly, it crashes. Does the shape makes sense to you? Is spectral axis supposed to be in arr.shape[2]? I still get confused by all the swapping that happens (astropy/specutils#923).

https://github.com/astropy/specutils/blob/7f7b55272b7e36d827c130bb2bfa0ec6078f41a3/specutils/spectra/spectrum1d.py#L265-L269

dhomeier · 2022-02-15T20:49:06Z

https://github.com/glue-viz/glue-astronomy/blob/7f3229d01bdc2bff64fdb1a83738447cf696c59d/glue_astronomy/translators/tests/test_spectrum1d.py#L204-#234
will need to be changed to flux.T/spec.flux.T if that is the intended behaviour now. Still feels a bit weird...

pllim · 2022-02-21T21:10:20Z

Is there a way to push this forward? I need this for spacetelescope/jdaviz#1040 .

dhomeier · 2022-02-21T22:31:34Z

I could go ahead with adapting the tests, but @astrofrog and I are quite uncertain at this point what the logic of reordering axes here again is. specutils as far as I see it is consistent in using the last dimension as spectral axis, so it's rather confusing to have the glue loader swap this again, and (only?) for [G]WCS spectra.

pllim · 2022-02-21T22:35:26Z

In our FITS data, the spectral axis seems to be in NAXIS3, which in Python-speak, is axis=0, so maybe it is specutils who is putting it in the wrong end. Though perhaps that is necessary for performance reason? 🤷

dhomeier · 2022-02-21T23:32:15Z

NAXIS3 -> axis=0 is just the conversion from Fortran- to C-ordering, right?
https://github.com/astropy/specutils/blob/af729120269e776e40d8a5e6c2e270617d7a8d6e/specutils/spectra/spectrum1d.py#L199-L205
notes that for the WCS the order is turned around again, but I think that is simply because by convention the WCS is kept in Fortran order. While the two have to match, I so not see a technical reason why the spectral axis has to be last, and performance-wise it would certainly be better to avoid those np.swapaxes.
I suppose the spectral axis was simply found more frequently in NAXIS1, and to avoid even more confusion that was set as a rule. According to the comment in the data_translator the convention for glue is exactly the opposite, so I guess swapping the axes again (and fixing the tests accordingly) is unavoidable at this point.
If the "glue-order" is common for large data cube, I think it is worth considering to allow that in specutils as well (e.g. MaNGA also uses NAXIS3, and the axes swapping seems to introduce a quite noticeable delay).

pllim · 2022-02-22T00:05:41Z

Yes, I think @rosteen mentioned that it would be nice if specutils is flexible enough to allow specutils in whatever axis but that's unlikely to happen any time soon. :(

dhomeier · 2022-02-22T01:06:42Z

glue_astronomy/translators/spectrum1d.py

-    def world_axis_names(self):
-        return (UCD_TO_SPECTRAL_NAME[self.spectral_wcs.world_axis_physical_types[0]], 'Offset')


This is changing the label of the returned components in the 2d example to 'World 0','World 1', breaking test_spectrum1d_2d_data further down. Maybe the new naming is more logical, but that should be documented somewhere (and at least World 1 does not sound like an improvement over Frequency).

dhomeier · 2022-02-22T14:02:58Z

@rosteen I have changed the expected labels in the test for now, so it can progress further, but does not necessarily mean they should remain like this.
The current failure however raises already when just dereferencing data['World 1'] as an array-like object ( in the call to _return_single_array), which seems to indicate something is still more fundamentally broken in the implementation (or is exposing a new bug in astropy.wcs?).

pllim · 2022-02-22T16:26:51Z

What does this error mean?

        assert_equal(data['World 0'], [[10, 10, 10], [10, 10, 10]])
>       assert data['World 1'].shape == (2, 3)

glue_astronomy/translators/tests/test_spectrum1d.py:255: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
.tox/py38-test/lib/python3.8/site-packages/glue/core/data.py:571: in __getitem__
    return self.get_data(key, view=view)
.tox/py38-test/lib/python3.8/site-packages/glue/core/data.py:1366: in get_data
    result = comp.data
.tox/py38-test/lib/python3.8/site-packages/glue/core/component.py:215: in data
    return self._calculate()
.tox/py38-test/lib/python3.8/site-packages/glue/core/component.py:313: in _calculate
    world_coords = pixel2world_single_axis(self._data.coords,
.tox/py38-test/lib/python3.8/site-packages/glue/core/coordinate_helpers.py:56: in pixel2world_single_axis
    result = wcs.pixel_to_world_values(*pixel)
.tox/py38-test/lib/python3.8/site-packages/ndcube/wcs/wrappers/compound_wcs.py:112: in pixel_to_world_values
    world_arrays_sub = w.pixel_to_world_values(*pixel_arrays_sub)
.tox/py38-test/lib/python3.8/site-packages/astropy/wcs/wcsapi/fitswcs.py:322: in pixel_to_world_values
    world = self.all_pix2world(*pixel_arrays, 0)

dhomeier · 2022-02-22T17:29:00Z

Per my speculation above, it's two levels further down ;-)

../../opt/lib/python3.9/site-packages/astropy/wcs/wcs.py:1351: in all_pix2world
    return self._array_converter(
../../opt/lib/python3.9/site-packages/astropy/wcs/wcs.py:1328: in _array_converter
    return _return_single_array(xy, origin)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

xy = array([[0, 0, 0]]), origin = 0

    def _return_single_array(xy, origin):
        if xy.shape[-1] != self.naxis:
>           raise ValueError(
                "When providing two arguments, the array must be "
                "of shape (N, {})".format(self.naxis))
E           ValueError: When providing two arguments, the array must be of shape (N, 1)

This apparently already when trying to access the shape attribute. Looks wicked. Or is it just that the shape is inconsistent with the no. of dimensions somewhere else?

pllim · 2022-02-23T15:23:00Z

Definitely something is still wonky here, unfortunately. Even if you ignore the data['World 1'] parts of the test (it seems to go into ndcube at some point and I got lost there), it crashes later at:

>>> s, o = data.coords.pixel_to_world(1, 2)
AttributeError: 'PaddedSpectrumWCS' object has no attribute 'pixel_to_world'

and

>>> px, py = data.coords.world_to_pixel(s, o)
AttributeError: 'PaddedSpectrumWCS' object has no attribute 'world_to_pixel'

The spec_new stuff works.

pllim · 2022-02-23T15:23:56Z

Maybe subclassing from CompoundLowLevelWCS is too low of a level?

dhomeier · 2022-02-23T15:35:11Z

Maybe subclassing from CompoundLowLevelWCS is too low of a level?

Yea, the pixel_to_world etc. are only provided by the HighLevel version, but that in turn is not supported by gwcs afair.
Dunno if it can all be moved to the pixel_to_world_values level...

pllim

As per PO feedback at sprint review today, they want the spectral axis to be dimensionless rather than pixels. Is it possible to do that upstream here? If not, I'll have to hack something up downstream in Jdaviz.

pllim · 2022-02-28T21:29:32Z

glue_astronomy/translators/spectrum1d.py

+    def __init__(self, spectral_wcs, n_extra_axes):
+        self.spectral_wcs = spectral_wcs
+        frame1 = CoordinateFrame(n_extra_axes, ['SPATIAL']*n_extra_axes,
+                                 np.arange(n_extra_axes), unit=[u.pix]*n_extra_axes,


Suggested change

np.arange(n_extra_axes), unit=[u.pix]*n_extra_axes,

np.arange(n_extra_axes), unit=[u.dimensionless_unscaled]*n_extra_axes,

pllim · 2022-02-28T21:29:49Z

glue_astronomy/translators/spectrum1d.py

+                                 np.arange(n_extra_axes), unit=[u.pix]*n_extra_axes,
+                                 name="Dummy1")
+        frame2 = CoordinateFrame(n_extra_axes, ['SPATIAL']*n_extra_axes,
+                                 np.arange(n_extra_axes), unit=[u.pix]*n_extra_axes,


Suggested change

np.arange(n_extra_axes), unit=[u.pix]*n_extra_axes,

np.arange(n_extra_axes), unit=[u.dimensionless_unscaled]*n_extra_axes,

rosteen · 2022-03-01T14:31:44Z

As per PO feedback at sprint review today, they want the spectral axis to be dimensionless rather than pixels. Is it possible to do that upstream here? If not, I'll have to hack something up downstream in Jdaviz.

I seem to remember a decision specifically not to handle dimensionless "spectral" axes in specutils, with the compromise that we'd handle pixel units. I could be remembering wrong though, I'll look back through some notes/git discussion.

Btw, thanks @dhomeier and @pllim for looking at this, I left it in a state where I'd run into the limit of my WCS knowledge. I'm trying to find time this sprint to get back to it.

pllim · 2022-03-01T20:20:23Z

dimensionless "spectral" axes

Okay, just keep it as pixel here. I dealt with that part downstream. Thanks!

pllim · 2022-03-22T19:06:07Z

This can be closed without merged. Superseded by #68

Fix bug in translating 3D Spectrum1D with only spectral axis defined …

6304448

…(rather than full WCS)

rosteen marked this pull request as draft January 25, 2022 16:30

rosteen added 3 commits January 26, 2022 10:44

Generalizing WCS padding with NDCube CompoundLowLevelWCS

4883dac

Fix codestyle checks

5481719

Swap spectral axis back to last manually if using PaddedSpectrumWCS

d772700

This comment was marked as resolved.

Sign in to view

pllim mentioned this pull request Jan 26, 2022

Cubeviz parser: Refactor parser, add ndarray support, metadata viewer spacetelescope/jdaviz#1040

Closed

27 tasks

rosteen mentioned this pull request Jan 27, 2022

Spectrum1D translator cannot handle spectral_axis in 3D cube #60

Closed

This comment was marked as resolved.

Sign in to view

pllim added a commit to pllim/jdaviz that referenced this pull request Feb 14, 2022

This works with glue-viz/glue-astronomy#61

3a9726a

pllim approved these changes Feb 14, 2022

View reviewed changes

pllim added a commit to pllim/jdaviz that referenced this pull request Feb 16, 2022

DO NOT MERGE: Use glue-viz/glue-astronomy#61

3b47227

pllim added a commit to pllim/jdaviz that referenced this pull request Feb 16, 2022

DO NOT MERGE: Use glue-viz/glue-astronomy#61

ef515b0

pllim mentioned this pull request Feb 16, 2022

spectral_slab crashes when spectral axis is in the unit of pixel astropy/specutils#924

Closed

pllim added a commit to pllim/jdaviz that referenced this pull request Feb 21, 2022

This works with glue-viz/glue-astronomy#61

21b0efb

pllim added a commit to pllim/jdaviz that referenced this pull request Feb 21, 2022

DO NOT MERGE: Use glue-viz/glue-astronomy#61

43a7ef1

dhomeier reviewed Feb 22, 2022

View reviewed changes

DNM: attempt to fix spectrum1d_2d; catch WCS warnings

312a840

dhomeier force-pushed the spec3d-bugfix branch from 85f121c to 312a840 Compare February 22, 2022 02:31

pllim added a commit to pllim/jdaviz that referenced this pull request Feb 23, 2022

This works with glue-viz/glue-astronomy#61

06e916b

pllim added a commit to pllim/jdaviz that referenced this pull request Feb 23, 2022

DO NOT MERGE: Use glue-viz/glue-astronomy#61

e7b2f6b

pllim added a commit to pllim/jdaviz that referenced this pull request Feb 23, 2022

This works with glue-viz/glue-astronomy#61

279b099

pllim added a commit to pllim/jdaviz that referenced this pull request Feb 23, 2022

DO NOT MERGE: Use glue-viz/glue-astronomy#61

1ff1325

pllim reviewed Feb 28, 2022

View reviewed changes

pllim added a commit to pllim/jdaviz that referenced this pull request Feb 28, 2022

This works with glue-viz/glue-astronomy#61

d57ba15

pllim added a commit to pllim/jdaviz that referenced this pull request Feb 28, 2022

DO NOT MERGE: Use glue-viz/glue-astronomy#61

d54462e

pllim added a commit to pllim/jdaviz that referenced this pull request Mar 1, 2022

This works with glue-viz/glue-astronomy#61

aaccc89

pllim added a commit to pllim/jdaviz that referenced this pull request Mar 1, 2022

DO NOT MERGE: Use glue-viz/glue-astronomy#61

d1b5ce1

pllim added a commit to pllim/jdaviz that referenced this pull request Mar 2, 2022

This works with glue-viz/glue-astronomy#61

7a4ea9a

pllim added a commit to pllim/jdaviz that referenced this pull request Mar 2, 2022

DO NOT MERGE: Use glue-viz/glue-astronomy#61

23493ec

pllim added a commit to pllim/jdaviz that referenced this pull request Mar 9, 2022

This works with glue-viz/glue-astronomy#61

85067a3

pllim added a commit to pllim/jdaviz that referenced this pull request Mar 9, 2022

DO NOT MERGE: Use glue-viz/glue-astronomy#61

ac1cf2c

pllim added a commit to pllim/jdaviz that referenced this pull request Mar 10, 2022

This works with glue-viz/glue-astronomy#61

a00ec51

pllim added a commit to pllim/jdaviz that referenced this pull request Mar 10, 2022

DO NOT MERGE: Use glue-viz/glue-astronomy#61

d014096

pllim added a commit to pllim/jdaviz that referenced this pull request Mar 11, 2022

This works with glue-viz/glue-astronomy#61

a0ecf32

pllim added a commit to pllim/jdaviz that referenced this pull request Mar 11, 2022

DO NOT MERGE: Use glue-viz/glue-astronomy#61

91c7af9

pllim added a commit to pllim/jdaviz that referenced this pull request Mar 15, 2022

This works with glue-viz/glue-astronomy#61

ba7a731

pllim added a commit to pllim/jdaviz that referenced this pull request Mar 15, 2022

DO NOT MERGE: Use glue-viz/glue-astronomy#61

481d49f

rosteen mentioned this pull request Mar 16, 2022

Generalize PaddedSpectrumWCS for higher dimensionality #68

Merged

astrofrog closed this Mar 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bug in translating 3D Spectrum1D with only spectral axis defined #61

Fix bug in translating 3D Spectrum1D with only spectral axis defined #61

rosteen commented Jan 25, 2022 •

edited

Loading

codecov bot commented Jan 25, 2022 •

edited

Loading

This comment was marked as resolved.

This comment was marked as resolved.

rosteen commented Jan 27, 2022 •

edited

Loading

This comment was marked as resolved.

pllim commented Feb 14, 2022 •

edited

Loading

dhomeier commented Feb 15, 2022

pllim commented Feb 21, 2022

dhomeier commented Feb 21, 2022

pllim commented Feb 21, 2022

dhomeier commented Feb 21, 2022 •

edited

Loading

pllim commented Feb 22, 2022

dhomeier Feb 22, 2022

dhomeier commented Feb 22, 2022

pllim commented Feb 22, 2022

dhomeier commented Feb 22, 2022

pllim commented Feb 23, 2022 •

edited

Loading

pllim commented Feb 23, 2022

dhomeier commented Feb 23, 2022

pllim left a comment

pllim Feb 28, 2022

pllim Feb 28, 2022

rosteen commented Mar 1, 2022

pllim commented Mar 1, 2022

pllim commented Mar 22, 2022

		def world_axis_names(self):
		return (UCD_TO_SPECTRAL_NAME[self.spectral_wcs.world_axis_physical_types[0]], 'Offset')

	np.arange(n_extra_axes), unit=[u.pix]*n_extra_axes,
	np.arange(n_extra_axes), unit=[u.dimensionless_unscaled]*n_extra_axes,

Fix bug in translating 3D Spectrum1D with only spectral axis defined #61

Fix bug in translating 3D Spectrum1D with only spectral axis defined #61

Conversation

rosteen commented Jan 25, 2022 • edited Loading

Description

codecov bot commented Jan 25, 2022 • edited Loading

Codecov Report

This comment was marked as resolved.

This comment was marked as resolved.

rosteen commented Jan 27, 2022 • edited Loading

This comment was marked as resolved.

pllim commented Feb 14, 2022 • edited Loading

dhomeier commented Feb 15, 2022

pllim commented Feb 21, 2022

dhomeier commented Feb 21, 2022

pllim commented Feb 21, 2022

dhomeier commented Feb 21, 2022 • edited Loading

pllim commented Feb 22, 2022

dhomeier Feb 22, 2022

Choose a reason for hiding this comment

dhomeier commented Feb 22, 2022

pllim commented Feb 22, 2022

dhomeier commented Feb 22, 2022

pllim commented Feb 23, 2022 • edited Loading

pllim commented Feb 23, 2022

dhomeier commented Feb 23, 2022

pllim left a comment

Choose a reason for hiding this comment

pllim Feb 28, 2022

Choose a reason for hiding this comment

pllim Feb 28, 2022

Choose a reason for hiding this comment

rosteen commented Mar 1, 2022

pllim commented Mar 1, 2022

pllim commented Mar 22, 2022

rosteen commented Jan 25, 2022 •

edited

Loading

codecov bot commented Jan 25, 2022 •

edited

Loading

rosteen commented Jan 27, 2022 •

edited

Loading

pllim commented Feb 14, 2022 •

edited

Loading

dhomeier commented Feb 21, 2022 •

edited

Loading

pllim commented Feb 23, 2022 •

edited

Loading