Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

adjusting links to enable pixel, wavelength linking in specviz2d #2736

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

gibsongreen
Copy link
Contributor

@gibsongreen gibsongreen commented Feb 29, 2024

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

Description:
some combination of the specviz2d wavelength axis and unit conversion work seems to have broken the handling of subsets between the 1d and 2d viewers.  If a subset is drawn in the 2d viewer and displayed in the subset plugin, the units are still reported as in wavelength units, which might be the reason they are not correctly translated onto the 1d viewer.

Validation Needed:
If a subset is created in the 2d viewer, is it translated correctly to the 1d (and vis versa)?

Bug Behavior:

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

Updated Behavior 1 (Subset Tools in µm irregardless of which viewer subset is created in):

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

Mosviz Behavior on Main:

Screen.Recording.2024-03-08.at.12.11.52.AM.mov

Mosviz Updated Behavior:

Screen.Recording.2024-03-08.at.12.16.32.AM.mov

Follow up:
Subset Tools for Mosviz, display units and values currently not displaying in plugin tab for subsets drawn in either viewer.
🐱

Drawing a subset in 2d viewer not displaying in either Mosviz Viewer in Mosviz.
🐱

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

@pllim pllim added this to the 3.8.3 milestone Feb 29, 2024
@pllim pllim added bug Something isn't working specviz2d 💤backport-v3.8.x on-merge: backport to v3.8.x labels Feb 29, 2024
@javerbukh
Copy link
Contributor

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.

image

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
Copy link
Member

kecnry commented Mar 8, 2024

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
Copy link
Contributor

@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
Copy link
Member

kecnry commented Mar 8, 2024

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
Copy link
Contributor Author

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.

image

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
Copy link
Contributor Author

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
Copy link
Member

kecnry commented Mar 11, 2024

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


# subset drawn in 1d viewer, want data in 2d viewer
viewer_1d.apply_roi(XRangeROI(.0001, .0002))
subset_drawn_1d = viewer_2d.native_marks[-1].image
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Getting the 3d image array this way is loading incorrectly only in the test. It should be (155, 864, 4) [which is how it loads in a Notebook], in the test file it is loading a zero array of shape (10,10,4), so the test thinks there's no mask. This is the last hold up.

@gibsongreen
Copy link
Contributor Author

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
Copy link
Contributor

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

@pllim
Copy link
Contributor

pllim commented Mar 14, 2024

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
Copy link
Member

kecnry commented Mar 14, 2024

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

@pllim
Copy link
Contributor

pllim commented Mar 14, 2024

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

@pllim
Copy link
Contributor

pllim commented Mar 14, 2024

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
Copy link
Member

kecnry commented Mar 14, 2024

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...

@pllim
Copy link
Contributor

pllim commented Mar 14, 2024

The job on main still passes, so I think failure is related to this PR and cannot be ignored.

@gibsongreen gibsongreen reopened this Mar 14, 2024
@gibsongreen
Copy link
Contributor Author

gibsongreen commented Mar 14, 2024

The job on main still passes, so I think failure is related to this PR and cannot be ignored.

I am going through it now with my env set to 3.11

@gibsongreen gibsongreen modified the milestones: 3.8.3, 3.10 Apr 5, 2024
@bmorris3 bmorris3 modified the milestones: 3.10, 3.11 May 3, 2024
@rosteen rosteen modified the milestones: 3.11, 4.1 Oct 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working specviz2d 💤backport-v3.8.x on-merge: backport to v3.8.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants