Update Mosviz to use Spectrum1D from SpectralCube

[duytnguyendtn]

This PR provides a first pass to trying to add Specutils support (Both Spectrum1D and SpectrumList) to our Mosviz parsers. It is blocked by #547. There are a few failure modes, mainly surrounding WCS indexing:

In glue core, axis name retrieval fails:
https://github.com/glue-viz/glue/blob/ad14432229255dc31c5756434cc18bf14657c92b/glue/core/coordinate_helpers.py#L167

Likewise:
https://github.com/glue-viz/glue/blob/ad14432229255dc31c5756434cc18bf14657c92b/glue/core/component.py#L220

I wonder if that means the WCS isn't being parsing correctly, but I'm going to need more help to diagnose this; I think this is as far as I can push solo.

Fix #837 (the part that will pin glue-astronomy to 0.3 or later; Tom R said he is going to put it in as part of this PR)

[astrofrog]

I've pushed a commit making use of the latest dev glue-astronomy - things aren't working quite right yet but at least the x and y world axes for the 2D spectrum are now correct by default (Wavelength and Offset). I'm running into issues related to the linking of the x axes of the 2D and 1D spectral viewers which I'll try and take a look at unless this is already being worked on.

[pllim]

I'm running into issues related to the linking of the x axes of the 2D and 1D spectral viewers which I'll try and take a look at unless this is already being worked on.

I am not working on anything like that myself. @rosteen , @javerbukh , or @ojustino ?

[astrofrog]

Ok actually the axis linking issue is not crucial to reviewing this in the end. I think this should now be ready for testing out - note that this needs the latest dev version of glue-astronomy. With this PR, the axes for the 2D spectra are as follows:

I haven't tested how the linking of datasets is working now - I think it would make sense to first wrap up #762 then I can rebase this on top of main and make sure the linking works properly then.

I don't expect this to work flawlessly at the moment as I haven't used MOSViz enough to know what is expected to work etc - so this would really benefit from some detailed testing from MOSViz experts to determine whether anything is broken compared to main.

Once we are happy with this, I can tag a release of glue-astronomy and pin the minimum version in jdaviz.

[astrofrog]

(I'll look into the test failures early next week but I don't think they are critical for testing this out)

[ojustino]

@astrofrog, I agree that it makes sense to wait for a decision to be made on #762, as that touches the linkage between the 1D and 2D spectrum viewers' x axes. We also have an open question there about whether to link either of them to the image viewer; given these axes, I think continuing to not do so makes sense.

I can review this in the next couple of days.

[astrofrog]

@ojustino - with this PR, the spatial axis of the 2D spectra is no longer RA/Dec so definitely can't be linked with the images. I think it only makes sense to link the spectral axes between all 1D and 2D spectra, and maybe the offset axis of the 2D spectra to other 2D spectra.

[astrofrog]

Note that this is still a fake spectral WCS, as it was before.

[duytnguyendtn]

Is there a reason why we still need to fake the WCS @astrofrog? Particularly with #735's strategy to link by pixel, I imagine a WCS, even for Mosviz, is no longer an absolute requirement? I think we were hoping that with transitioning to Spectrum1D, we could drop these fake values?

My understanding was that the WCS was required to construct a SpectralCube, hence why faking these values were required. I don't believe Spectrum1Ds have this requirement?

[astrofrog]

Yes we could just not add a WCS and link by pixels in this case.

[duytnguyendtn]

It sounds in offband conversations, the Product Owners are favoring just pushing this through and being content that we reduced our hardcoded values to only the NIRISS case. It seems Pixel Linking will only stall this PR further, so we can tackle this improvement at a later date

[astrofrog]

I think the remaining CI failures may be fixed once glue-astronomy is released.

[ojustino]

@astrofrog, I was able to test Mosviz with this branch and the latest version of glue-astronomy. I didn't encounter anything strange with data loading, subset selection, or model fitting compared to main, and I do see the new axis in the Viewer menu.

[astrofrog]

Great! So maybe the best path forward is to merge #762, rebase this, release glue-astronomy then update this PR to update the minimum version of glue-astronomy - and then make sure all the subset stuff works as expected before merging this.

[astrofrog]

I've now updated this to use the latest glue-astronomy version, and have started to investigate subset issues:

• When selecting subsets in a 1D spectrum, an error used to occur which meant the subset was not immediately shown but required changing object and changing back. This should be fixed by Improve subset handling glue-viz/glue-jupyter#256 and I have released glue-jupyter 0.8.1 with this fix.

• When switching to a different 1D spectrum, the subset was not shown - it was only shown for the original spectrum the selection was made in. This is because the linking in Link mosviz data as a batch after they are loaded #762 only links the spectral axis of each 1D spectrum with its corresponding 2D spectrum but didn't link each 1D spectrum to each other 1D spectrum. I've now fixed this in ab0e243 in this PR.

• When making a selection in a 1D spectrum, the corresponding region is not shown in the 2D spectrum. There is something funky going on with the spectral WCS of the Spectrum1D objects, which I will investigate next. Once I have fixed this, the subset propagation from 1D to 2D should work.

• When making a selection in a 2D spectrum, the corresponding region is not shown in the 1D spectrum. This is expected, as the subsets made in the 2D viewer are 2D and can't be applied to the 1D spectrum conceptually. For instance, if I only select a rectangular region that only covers a narrow range of offsets, should the 1D spectrum show the corresponding region as highlighted or not?

Are there other known subset issues?

In addition to this, the slit information is not being parsed by Spectrum1D so I will need to investigate the best way to add this back in.

[pllim]

Whoops... I think I might have caused the conflict. Can you please rebase? Thanks!

[astrofrog]

I believe this is now ready for review and final testing!

The slit overlay works again for NIRCAM data, and I have implemented a fix for the loading of the NIRISS 2D spectrum wavelength information. I have not implemented any kind of fallback to pixel linking for now as this is beyond the scope of this PR. With this PR as-is, functionality should be the same as before but with many of the subset issues hopefully fixed and with more sensible axes for the 2D spectra:

An interesting effect that you might see is the following:

This is 'correct' because the 2D spectra are actually in reverse wavelength order. This can be fixed in the 2D spectrum viewer if desired, but also beyond the scope of this PR.

[codecov]

Codecov Report

Merging #733 (1cc55d1) into main (a9236cf) will increase coverage by 0.30%.
The diff coverage is 74.41%.

@@            Coverage Diff             @@
##             main     #733      +/-   ##
==========================================
+ Coverage   68.13%   68.43%   +0.30%     
==========================================
  Files          65       65              
  Lines        4767     4743      -24     
==========================================
- Hits         3248     3246       -2     
+ Misses       1519     1497      -22     

Impacted Files	Coverage Δ
jdaviz/configs/mosviz/plugins/parsers.py	79.06% <73.80%> (+4.13%)	⬆️
jdaviz/configs/mosviz/plugins/viewers.py	74.25% <100.00%> (-2.11%)	⬇️
jdaviz/app.py	82.66% <0.00%> (-0.23%)	⬇️
...configs/default/plugins/data_tools/file_chooser.py	69.14% <0.00%> (+3.42%)	⬆️

---

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 a9236cf...1cc55d1. Read the comment docs.

[pllim]

Does this mean we can remove spectral_cube from the dependency list too?

[rosteen]

Does this mean we can remove spectral_cube from the dependency list too?

Not until we also merge @javerbukh's PR removing it from Cubeviz.

[pllim]

Which PR is that?

[javerbukh]

@pllim #547 , but that PR may need to be reworked since it is about 5 months old.

[rosteen]

Looks good to me, thanks! We'll definitely have to double check that everything works with #852 after whichever one gets merged first is in.

[pllim]

Who has better rebase skills: @astrofrog or @mariobuikhuizen ? 😆

[javerbukh]

Looks good and works as expected!

[astrofrog]

Merge in numerical order? 😂