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

Hide spectral subsets in cubeviz #2636

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

gibsongreen
Copy link
Contributor

@gibsongreen gibsongreen commented Dec 22, 2023

Description

Screen.Recording.2023-12-19.at.12.03.35.PM.mov

Behavior in the video displays that the initial overlay when a slice is viewed is still visible in flux/uncert viewers. After a state change, overlays become hidden.

When _expected_subset_layer_default() is called in on_layers_update(), the subset is either created and but the data is inaccessible, or the glue message is stating that the subset is in the process of being create, in either case, resulting in type None for data contained within the subset.

Looking into the message created from _on_subset_create(), yields the same subset data of type None. Tracing through the remaining default viewer class methods yields the same conclusion.

Note:

  • By inferring the subset type is possible for the uncert-viewer, this can be done by accessing the data label, and using the string ending to turn off this overlay at the beginning, while still enabling spatial subsets to be shown on the viewer.
  • The same assumption may be possible with the flux-viewer, although, the SCI data is used for both flux & spectrum-viewers, and so the share the same label. If there was a separate differentiation, then this may also be possible.

Fixes #

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.
  • After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

@kecnry
Copy link
Member

kecnry commented Dec 22, 2023

By inferring the subset type is possible for the uncert-viewer, this can be done by accessing the data label, and using the string ending to turn off this overlay at the beginning, while still enabling spatial subsets to be shown on the viewer.

I don't think this is reliable enough - we do allow loading the flux cube into the uncertainty viewer (and vice versa).

If its not possible to access the information we need at available callbacks/methods, then I think we should make an upstream request for a message to be sent after a subset is created and fully available, and then make use of that here.

Copy link

codecov bot commented Dec 22, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (09d7da6) 91.52% compared to head (cf3d3f5) 91.52%.
Report is 6 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2636   +/-   ##
=======================================
  Coverage   91.52%   91.52%           
=======================================
  Files         161      161           
  Lines       19969    19982   +13     
=======================================
+ Hits        18277    18289   +12     
- Misses       1692     1693    +1     

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

@pllim pllim added this to the 3.9 milestone Dec 22, 2023
@pllim pllim added the cubeviz label Dec 22, 2023
@gibsongreen
Copy link
Contributor Author

By inferring the subset type is possible for the uncert-viewer, this can be done by accessing the data label, and using the string ending to turn off this overlay at the beginning, while still enabling spatial subsets to be shown on the viewer.

I don't think this is reliable enough - we do allow loading the flux cube into the uncertainty viewer (and vice versa).

If its not possible to access the information we need at available callbacks/methods, then I think we should make an upstream request for a message to be sent after a subset is created and fully available, and then make use of that here.

I was thinking that it was a pretty hard-coded way of doing too so I didn't include it in the file changes nor demo it, and now that you mention it, the vis versa, was one thing that I was doing to test the state changes.

All in all I think the upstream change like we talked about in code review is the best past forward.

@gibsongreen gibsongreen marked this pull request as draft December 22, 2023 15:03
@gibsongreen gibsongreen modified the milestones: 3.9, 3.10 Apr 5, 2024
@dhomeier
Copy link
Collaborator

@gibsongreen do you know if your subsets are created directly via
https://github.com/glue-viz/glue/blob/1981b686b3ba9df86a45e8cf00d190cf09e0fbe6/glue/core/data.py#L214
or using new_subset_group as indicated in the docstring there?

@bmorris3 bmorris3 modified the milestones: 3.10, 3.11 May 3, 2024
@gibsongreen
Copy link
Contributor Author

@gibsongreen do you know if your subsets are created directly via https://github.com/glue-viz/glue/blob/1981b686b3ba9df86a45e8cf00d190cf09e0fbe6/glue/core/data.py#L214 or using new_subset_group as indicated in the docstring there?

Hey @dhomeier , it does not appear that we use add_susbset()within jdaviz, we do seem to use new_subset_group

@rosteen rosteen modified the milestones: 3.11, 4.1 Oct 17, 2024
@rosteen rosteen modified the milestones: 4.1, 4.2 Dec 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants