exclude WCS-only layer subsets from plot options #2699

kecnry · 2024-02-12T19:24:45Z

Description

This pull request filters out the wcs-only subset layer (when linked by WCS in imviz) for the subset visibility toggle as well as in the visibility mixed-state logic in the viewer tabs.

Previously:

Screen.Recording.2024-02-12.at.2.23.20.PM.mov

After this PR:

Screen.Recording.2024-02-12.at.3.07.05.PM.mov

Note that the first click is still showing mixed state - that is a broader bug than the scope of this effort. I did some investigation, and PlotOptionsSyncState is requesting that both be updated to be hidden and its not clear where that bug is coming from.

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

* both sync state and the visibility mixed state for the viewer select tabs

kecnry · 2024-02-12T20:26:05Z

jdaviz/core/template_mixin.py

                    layer.add_callback('color', self._update_layer_items)
                    layer.add_callback('visible', self._update_layer_items)
-                    break


this had to be removed in order for mixed state of new subsets to be shown correctly on the tabs

kecnry · 2024-02-12T20:26:27Z

jdaviz/core/template_mixin.py

-        def is_wcs_only(layer):
-            return getattr(layer.layer, 'meta', {}).get(self.app._wcs_only_label, False)


now use the consolidated version imported from utils

kecnry · 2024-02-12T20:26:55Z

jdaviz/core/template_mixin.py

-        elif isinstance(self.value, (int, float)) and self._glue_name != 'percentile':
+        elif self._glue_name != 'percentile' and isinstance(self.value, (int, float)):


technically not related to this effort, but a small cleanup that will avoid the isinstance for all cases that aren't percentile.

kecnry · 2024-02-12T20:27:20Z

jdaviz/utils.py

+_wcs_only_label = "_WCS_ONLY"
+
+
+def is_wcs_only(layer):
+    # exclude WCS-only layers from the layer choices:
+    if hasattr(layer, 'layer'):
+        state = layer.layer
+    elif hasattr(layer, 'data'):
+        state = layer.data
+    elif hasattr(layer, 'meta'):
+        state = layer
+    else:
+        raise NotImplementedError
+    return getattr(state, 'meta', {}).get(_wcs_only_label, False)
+
+
+def is_not_wcs_only(layer):
+    return not is_wcs_only(layer)
+
+


consolidate all this logic in one place and move _wcs_only_label outside app so that these can be functions instead of methods.

pllim

I like all the clean-ups and videos demonstrated the fix. Thanks!

Not sure if you also want to do something about this or not, but it is not related to this PR:

jdaviz/jdaviz/configs/imviz/wcs_utils.py

Line 364 in 54e1691

wcs_only_key="_WCS_ONLY", data=None,

kecnry · 2024-02-13T18:00:56Z

do you mean defaulting to _wcs_only_label instead of the hardcoded string? I think that could easily belong here if that makes sense to you and @bmorris3 doesn't see any reason not to do so.

pllim · 2024-02-13T18:04:04Z

Re: #2699 (comment)

As long as Python allows that, I think it is okay to change. I know there are some limitations to class API but this is a function, so should be ok?

codecov · 2024-02-13T18:14:40Z

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (fb6d841) 90.83% compared to head (b645527) 90.81%.
Report is 1 commits behind head on main.

Files	Patch %	Lines
jdaviz/utils.py	66.66%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2699      +/-   ##
==========================================
- Coverage   90.83%   90.81%   -0.03%     
==========================================
  Files         163      163              
  Lines       21502    21517      +15     
==========================================
+ Hits        19532    19541       +9     
- Misses       1970     1976       +6

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

gibsongreen

The video demo and local testing matches up, and the code changes look good to me!

kecnry added this to the 3.9 milestone Feb 12, 2024

github-actions bot added imviz plugin Label for plugins common to multiple configurations labels Feb 12, 2024

kecnry force-pushed the subset-visible-wcs-only branch from 4e5e2a6 to c624fdc Compare February 12, 2024 19:38

exclude WCS-only layer subsets from plot options

4de1879

* both sync state and the visibility mixed state for the viewer select tabs

kecnry force-pushed the subset-visible-wcs-only branch from c624fdc to 4de1879 Compare February 12, 2024 20:08

kecnry added 2 commits February 12, 2024 15:18

small code cleanup

e60607f

add to image rotation changelog entry

b645527

kecnry commented Feb 12, 2024

View reviewed changes

kecnry marked this pull request as ready for review February 12, 2024 20:27

kecnry requested review from rosteen, javerbukh, pllim, bmorris3, haticekaratay, cshanahan1 and gibsongreen as code owners February 12, 2024 20:27

pllim approved these changes Feb 13, 2024

View reviewed changes

kecnry added the Ready for final review label Feb 13, 2024

gibsongreen approved these changes Feb 14, 2024

View reviewed changes

kecnry merged commit bf46c9b into spacetelescope:main Feb 14, 2024
17 of 19 checks passed

kecnry deleted the subset-visible-wcs-only branch February 14, 2024 18:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

exclude WCS-only layer subsets from plot options #2699

exclude WCS-only layer subsets from plot options #2699

kecnry commented Feb 12, 2024 •

edited

Loading

kecnry Feb 12, 2024

kecnry Feb 12, 2024

kecnry Feb 12, 2024

kecnry Feb 12, 2024

pllim left a comment

kecnry commented Feb 13, 2024

pllim commented Feb 13, 2024

codecov bot commented Feb 13, 2024

gibsongreen left a comment

		def is_wcs_only(layer):
		return getattr(layer.layer, 'meta', {}).get(self.app._wcs_only_label, False)

		elif isinstance(self.value, (int, float)) and self._glue_name != 'percentile':
		elif self._glue_name != 'percentile' and isinstance(self.value, (int, float)):

exclude WCS-only layer subsets from plot options #2699

exclude WCS-only layer subsets from plot options #2699

Conversation

kecnry commented Feb 12, 2024 • edited Loading

Description

Change log entry

Checklist for package maintainer(s)

kecnry Feb 12, 2024

Choose a reason for hiding this comment

kecnry Feb 12, 2024

Choose a reason for hiding this comment

kecnry Feb 12, 2024

Choose a reason for hiding this comment

kecnry Feb 12, 2024

Choose a reason for hiding this comment

pllim left a comment

Choose a reason for hiding this comment

kecnry commented Feb 13, 2024

pllim commented Feb 13, 2024

codecov bot commented Feb 13, 2024

Codecov Report

gibsongreen left a comment

Choose a reason for hiding this comment

kecnry commented Feb 12, 2024 •

edited

Loading