Fix Spectrum1D translator handling of 1D spectra with spectral-only axes

[bmorris3]

The glue-astronomy translator generates glue Data objets from Spectrum1D with special treatment of the WCS coordinates. The current translator only produces a coordinate via SpectralCoordinates in the case where there's:

1. a 1D spectrum with 1D fluxes
2. WCS with one world dimension
3. and the WCS is an instance of gwcs.WCS.

I'm working with a use-case (https://github.com/spacetelescope/jdaviz/pull/2039) with a single non-gwcs spectral axis. In this PR, I've simply allowed non-gwcs axes to pass through if it is clearly marked as a spectral axis, and meets criteria 1 and 2.

[codecov[bot]]

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.01 :warning:

Comparison is base (478451f) 97.34% compared to head (a23544c) 97.34%.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #88 +/- ## ========================================== - Coverage 97.34% 97.34% -0.01% ========================================== Files 18 18 Lines 1355 1354 -1 ========================================== - Hits 1319 1318 -1 Misses 36 36 ``` | [Impacted Files](https://codecov.io/gh/glue-viz/glue-astronomy/pull/88?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glue-viz) | Coverage Δ | | |---|---|---| | [glue\_astronomy/translators/spectrum1d.py](https://codecov.io/gh/glue-viz/glue-astronomy/pull/88?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glue-viz#diff-Z2x1ZV9hc3Ryb25vbXkvdHJhbnNsYXRvcnMvc3BlY3RydW0xZC5weQ==) | `94.63% <100.00%> (-0.04%)` | :arrow_down: | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glue-viz). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glue-viz)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

[dhomeier]

Thanks for the PR, that functionality should definitely be supported. I was and still am concerned about the different treatment for the various data/wcs cases. Currently these cases are handled as

1. >1D flux (spectrum.data), >1D WCS: data.coords = spectrum.wcs (no further treatment).
2. >1D flux, 1D WCS: data.coords = PaddedSpectrumWCS() (custom expansion of spectral axis).
3. 1D flux, 1D WCS: data.coords = spectrum.wcs (no further treatment).
4. 1D flux, 1D GWCS: data.coords = SpectralCoordinates() (GWCS constructed from given spectral_axis).

So in case 4 coords directly provide a SpectralCoordinates object, while in 2 they return a PaddedSpectrumWCS containing SpectralCoordinates in its first element. They all should also provide pixel_to_world_values, while case 2 in addition provides pixel_to_world. Case 3 with the present changes would follow case 4, but its WCS could in fact just as well be used to construct a PaddedSpectrumWCS – with the padding of course not really necessary here, but offering all the functionality of case 2. There is a point for analogy to 4, both being fully 1D, but as both 2 and 3 are FITS WCS, it might also make sense to treat them as much alike as possible. I would thus normally tend to hand this to the PaddedSpectrumWCS branch (it may even be possible to wrap the GWCS case from 4 into one as well). But depending on the details of your use case and what functionality exactly you need from data.coords, we may still consider this (present) approach.

[bmorris3]

@dhomeier My understanding is that glue translators exist to get common domain-specific data structures constructed from glue-specific data structures. I'm not aware of uses for PaddedSpectrumWCS outside of glue-based applications, since this object is only a workaround to support limitations of Data.coords and ND spectra, as the source comments indicate:

https://github.com/glue-viz/glue-astronomy/blob/b5f2bf021c6665dcb134495ad02d3da3bd07c920/glue_astronomy/translators/spectrum1d.py#L37-L39

A search of all GitHub shows that PaddedSpectrumWCS is only used within glue-astronomy and jdaviz. I don't think many/any users will benefit from translating to this glue-specific data structure.

[dhomeier]

A search of all GitHub shows that PaddedSpectrumWCS is only used within glue-astronomy and jdaviz. I don't think many/any users will benefit from translating to this glue-specific data structure.

But they may e.g. from pixel_to_world. I agree that SpectralCoordinates is the object of interest, so it just comes down to where to locate it. Looking at spacetelescope/jdaviz#2039 it does already check the type of data.coords, so it probably should make no difference there.

[pllim]

Re: PaddedSpectrumWCS

I agree this should not be the norm. I would love that everyone not use it at all if we had that option.

[dhomeier]

You mean have glue just work with 1D spectral WCS for higher-dimensional data? I guess that would be desirable, but outside of glue-astronomy. How do you think about the use/availability of High Level WCS within coords?

[pllim]

If you are asking me, I am not sure. I don't deal with spectra that often. If data.coords could be completely APE 14 compatible, that would be the best but then again I am not sure if APE 14 alone is sufficient for all the spectra requirements, if I remember some past conversations in other repos correctly. Would have to rope in people like Tom R, Stuart M, and Adam G for that conversation.

[astrofrog]

I think glue should strictly follow APE 14 (and it should currently do that). I think the padded class is needed because specutils is giving a WCS which has different dimensionality to the data? (On phone so can't check)

[dhomeier]

Yes, PaddedSpectrumWCS is needed for >1D flux; it is not needed for the 1D flux array here, but would make the interface more similar to the multi-D case and provide high-level WCS functionality that SpectralCoordinates(spectral_axis) does not.

[bmorris3]

I think the padded class is needed because specutils is giving a WCS which has different dimensionality to the data? (On phone so can't check)

That's right. I think the remaining clarification needed here from @astrofrog is what type of coordinate object should be returned by glue-astronomy. PaddedSpectrumWCS provides compatibility between specutils and glue, which have different requirements for the number of specified coordinate dimensions. I'm advocating for a translator that returns the coordinates required for specutils, rather than glue.

[dhomeier]

I'm advocating for a translator that returns the coordinates required for specutils, rather than glue.

Not sure I understand – the former should be provided by to_object, and does either way in https://github.com/glue-viz/glue-astronomy/blob/b5f2bf021c6665dcb134495ad02d3da3bd07c920/glue_astronomy/translators/spectrum1d.py#L187-L190

to_data has to return Data.coords in the form glue can use them, right?

[bmorris3]

@dhomeier You're right, sorry for my confusion.

[astrofrog]

Coming back to this, I'm trying to understand why the changes here are needed - @bmorris3 could you provide some examples where this improves the behavior?

I'll note that SpectralCoordinates was created prior to glue adopting APE 14, and in fact it should no longer be needed so we should ideally try and phase it out if we can unless there is a good use case for it. The main issue with that class is that it simply tabulates the spectral coordinates and loses information about the actual WCS transformation. At the moment it seems that this PR actually makes more cases return SpectralCoordinates but I don't think we want that.

[bmorris3]

I think what's on main now is sufficient. Closing.