adjusting links to enable pixel, wavelength linking in specviz2d

[gibsongreen]

This pull request is to address subset linking in specviz2d 🐱.

Description:
This update updates LinkSame to LinkSameWithUnits, and appropriate components depending on the spectrum dimensionality is handled so subsets load within the bounds of both viewers, irregardless of which they were created in, nor if additional data items are added to the data collection (eg. via Spectral Extraction Plugin).

These updates should also ensure that unit handling for subsets is occurring. Unit changes can be seen in the plugin.

Note:

• If using specviz2d.app.get_subsets(use_display_units=True) needs the use_display_units argument.

Bug Behavior:

Screen.Recording.2024-03-01.at.3.08.53.PM.mov

Updated Behavior:

Screen.Recording.2025-02-04.at.9.46.05.AM.mov

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)?

[javerbukh]

I was able to draw subset 1 on the spectrum viewer and subset 2 on the 2d viewer and they appear at the right locations, but the units for subset 2 show up as um when I run specviz2d.app.get_subsets() instead of pix.

Looks like the reason for that is the get_subsets method assumes an xrange roi is spectral and thus applies the spectral units to it. We may need to add a check if the spectral subset was created on a 2d viewer or spectrum viewer.

jdaviz/jdaviz/app.py

Lines 943 to 945 in c731eef

	ret_units = self._get_display_unit('spectral')
	subset_bounds = [(subset_state.lo * units).to(ret_units, u.spectral()),
	(subset_state.hi * units).to(ret_units, u.spectral())]

[kecnry]

but the units for subset 2 show up as um when I run specviz2d.app.get_subsets() instead of pix.

I thought we had decided that was the intended behavior, at least for now until we have API support for requesting specific units?

[javerbukh]

@kecnry Its not scientifically correct though right? Totally fine to push that change to a future PR, just something I noticed. Otherwise this PR is looking good! It works with editing the subsets and creating composite ones. There are enough error messages that a user shouldn't be surprised by the current units behavior so feel free to ignore that comment for now.

[kecnry]

ah, sorry, I missed the screenshot. Yes, it should show the actual wavelength values (along with wavelength units), but definitely not mixed with wavelength values and pixel units. If that is too difficult for now, then we can also return pixel values and units when the subset was first created in the 2d viewer.

[gibsongreen]

I was able to draw subset 1 on the spectrum viewer and subset 2 on the 2d viewer and they appear at the right locations, but the units for subset 2 show up as um when I run specviz2d.app.get_subsets() instead of pix.

Looks like the reason for that is the get_subsets method assumes an xrange roi is spectral and thus applies the spectral units to it. We may need to add a check if the spectral subset was created on a 2d viewer or spectrum viewer.

jdaviz/jdaviz/app.py

Lines 943 to 945 in c731eef

	ret_units = self._get_display_unit('spectral')
	subset_bounds = [(subset_state.lo * units).to(ret_units, u.spectral()),
	(subset_state.hi * units).to(ret_units, u.spectral())]

I meant to get back sooner I was deep in testing-land. μm, the unit is appearing in Subset Tools irregardless of what viewer the subset is drawn in. So the correct value is showing up independent of the viewer, but the units are always appearing in μm.

In the case of the screenshot, if Subset 1 was drawn in the 1d viewer, and Subset 2d in the 2d viewer, are the values incorrect?

[gibsongreen]

Also, I split up the test into two temporarily. I'm going to have a look over the weekend. Drawing in 1d is working as expected for the test. Drawing in 2d, I am able to use the functionality in the test in a notebook, and it get the expected results.

When I load the image from the spectrum in the test, the image is loading with 0s, and the shape of the image is not as it should be.

[kecnry]

Getting the spectral display units is not the right assumption here if the subsets can be returned in pixels, but if that was the behavior in main, then can probably be considered a follow-up effort (please create a ticket).

[gibsongreen]

Getting the spectral display units is not the right assumption here if the subsets can be returned in pixels, but if that was the behavior in main, then can probably be considered a follow-up effort (please create a ticket).

Will do right now!

[javerbukh]

Do you have an idea of why the python 3.11 CI test is failing?

[pllim]

Looks like this patch broke unit handling somewhere.

pext.export_bg_sub(add_data=True)

specviz2d/plugins/spectral_extraction/tests/test_spectral_extraction.py:170

astropy.units.core.UnitConversionError: 'um' (length) and '' (dimensionless) are not convertible

[kecnry]

The dev tests are failing everywhere and we have a tech debt ticket to fix that ASAP 🐱

[pllim]

It is not a devdeps job. But it does pull in remote data.

[pllim]

Are you saying this is affecting main? I restarted an old successful job on main from an hour ago. Let's see.

https://github.com/spacetelescope/jdaviz/actions/runs/8286222588

[kecnry]

I thought it was affecting other PRs, but maybe not, I do see it passing on others. And since its specviz2d spectral extraction, the bug could be here...

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.53%. Comparing base (937e2bf) to head (540fa64).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2736   +/-   ##
=======================================
  Coverage   87.53%   87.53%           
=======================================
  Files         128      128           
  Lines       19957    19964    +7     
=======================================
+ Hits        17469    17476    +7     
  Misses       2488     2488           

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

[javerbukh]

Not able to have subsets appear in the other viewer. On a fresh environment in the Specviz2D notebook.

[gibsongreen]

Not able to have subsets appear in the other viewer. On a fresh environment in the Specviz2D notebook.

@javerbukh is the the 1D spectrum auto-extracted from the 2d spectrum with the data you used or did you load the 1D and 2D separately using load_data()?

Do you mind sending me the MAST link or data that you were testing with?

[javerbukh]

Can this commented out code be removed?

[gibsongreen]

That test is intentionally commented out for now until the SubsetCreateMessage PR is merged (which I believe we got an update this morning that it will be occurring soon). We should test that subsets created in either viewer loaded in the opposing viewer as well (but this at the moment is prevented with the Glue maxpin)

[javerbukh]

The bug I was encountering was because I unsuccessfully loaded MAST data into the viewer (the URI would error and no data would be retrieved) and then tried to load local data using the import button without restarting the kernel. @gibsongreen is investigating this workflow but otherwise this works well!