Fix cubeviz test broken by glue 1.19

[pllim]

Description

I think the test passed now but the test case described in #2805 (comment) is still broken. I got the image to display but both mouseover and slice are not working, so I think there might be more things broken than #2811 here but I don't know how to fix them.

Fixes #2811

Change log entry

• Is a change log needed? If yes, is it added to CHANGES.rst? If you want to avoid merge conflicts,
  list the proposed change log here for review and add to CHANGES.rst before merge. If no, maintainer
  should add a no-changelog-entry-needed label.

Checklist for package maintainer(s)

This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.

• Are two approvals required? Branch protection rule does not check for the second approval. If a second approval is not necessary, please apply the trivial label.
• Do the proposed changes actually accomplish desired goals? Also manually run the affected example notebooks, if necessary.
• Do the proposed changes follow the STScI Style Guides?
• Are tests added/updated as required? If so, do they follow the STScI Style Guides?
• Are docs added/updated as required? If so, do they follow the STScI Style Guides?
• Did the CI pass? If not, are the failures related?
• Is a milestone set? Set this to bugfix milestone if this is a bug fix and needs to be released ASAP; otherwise, set this to the next major release milestone. Bugfix milestone also needs an accompanying backport label.
• After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.79%. Comparing base (a6de416) to head (c60260f).
Report is 6 commits behind head on main.

❗ Current head c60260f differs from pull request most recent head 0395eda. Consider uploading reports for the commit 0395eda to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2812      +/-   ##
==========================================
- Coverage   88.82%   88.79%   -0.03%     
==========================================
  Files         111      110       -1     
  Lines       16879    16614     -265     
==========================================
- Hits        14993    14753     -240     
+ Misses       1886     1861      -25     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[kecnry]

is the bare exception here safe or could it get us until trouble in any way? Can we do something to check in advance instead or check for a specific expected exception?

[pllim]

The original traceback is very confusing so I don't know what is the exact exception to catch here. In general, I think falling back to not using spectral axis should be okay when Spectrum1D conversion fails?

[kecnry]

so the consequence of the exception being triggered (and returning an empty list) is just that that data entry will not show any valid units to convert?

[pllim]

Let's ask @javerbukh since I am not very sure where the output goes. Either that or it only uses basic equivalencies that might succeed or not.

[javerbukh]

This is used when data with a different spectral axis unit from the display data is visualized simultaneously with the display data in a viewer. For example data 1 with um is loaded and data 2 with AA is added, to_unit will convert the data from data 2 so that it matches the units of data 1. An empty list may result in the data not being visualized at all, so we should check that that actually happens and add a snackbar or other message so the user understands why the data was not added.

That said, if spec = data.get_object(cls=Spectrum1D) is throwing an error, something else seems to be broken and this should just temporarily get us around that issue.

TLDR: This looks like a good temporary fix but we should track the larger issue of why this is throwing an exception.

[kecnry]

I'm just worried that any future bug within get_object will be very hard to track down with this blanket exception 🤔

[pllim]

Should I close this without merge then, and someone else can pick it up next sprint for a proper fix?

[kecnry]

seems to be RuntimeError... but I'll look into if we can feasilby predict in advance instead of relying on try/except

[pllim]

@rosteen , for completeness, with a fresh env and forcing notebook to upgrade via pip (conda ships an older version), with this PR, mouseover and slice now work for me too.

But sounds like we're doing a proper fix, so I am closing this PR without merge.

Thanks, all!

[kecnry]

Here is a snippet of the traceback before the RuntimeError is raised:

../../../repos/glue/glue/core/units.py:31: in to_unit
    return self.converter_helper.to_unit(data, cid, values, original_units, target_units)
../../../repos/jdaviz/jdaviz/app.py:99: in to_unit
    spec = data.get_object(cls=Spectrum1D)
../../../repos/glue/glue/core/data.py:298: in get_object
    return handler.to_object(self, **kwargs)
../../../miniconda3/envs/jdaviz-dev/lib/python3.11/site-packages/glue_astronomy/translators/spectrum1d.py:287: in to_object
    return Spectrum1D(**data_kwargs, **kwargs)
../../../miniconda3/envs/jdaviz-dev/lib/python3.11/site-packages/specutils/spectra/spectrum1d.py:277: in __init__
    if hasattr(self.wcs, "spectral"):
../../../repos/astropy/astropy/wcs/wcs.py:3463: in spectral
    return self.sub([WCSSUB_SPECTRAL])  # Defined by C-ext
../../../repos/astropy/astropy/wcs/wcs.py:656: in sub
    copy = self.deepcopy()

I would argue that either glue-astronomy or specutils should be able to catch that this is an incompatible WCS and raise a useful exception that we can catch here (although a RuntimeError for now is probably worth just getting in?). The alternative would be to inspect the data entry within glue to determine if its compatible, but that might mean making assumptions about components/shapes/etc that maybe we should avoid? 🤷‍♂️

If everyone else is okay with it, I think we should get this PR in as it was but with RuntimeError instead of a catch-all, and then open follow-up tickets for better error handling upstream in either of those two packages.

[pllim]

When astropy/astropy#16298 is resolved (no ETA), the RuntimeError might become something else but shouldn't be hard to patch, so 👍 to catching RuntimeError for now. Thanks!

That said... @rosteen , do you know why specutils checks for if hasattr(self.wcs, "spectral") but that isn't enough?

[kecnry]

and why is hasattr triggering a deepcopy (which is ultimately what fails)?

[pllim]

Re: #2812 (comment)

I think that is why Tom R opened the astropy issue.

[rosteen]

That said... @rosteen , do you know why specutils checks for if hasattr(self.wcs, "spectral") but that isn't enough?

I had thought that the issue here was that we were trying to send non-spectral data through the Spectrum1D translator from Jdaviz, since we only check for a flux attribute there and there can be data (e.g. 2D data) that has flux but not a spectral axis. Is the data triggering this actually a spectral data cube?

[kecnry]

The WCS does suggest that it is a cube with a spectral axis - so maybe this is actually just a specutils bug in this case? And we just weren't ever sending this test data through unit conversion before and so didn't notice it was failing here until the glue changes forced the code to go through this logic for non-unit-conversion reasons?

[kecnry]

This is coming up more and more though - maybe we should just merge (including backporting) with either the RuntimeError or catch-all Exception and investigate further after?

[pllim]

Just speaking for the test case, I think the problem is that it is a actual spectral cube but data.coords becomes a 1D SpectralGWCS and that crashed glue-core>=0.19.