generalize subset select component to handle spatial and spectral #1175

kecnry · 2022-03-17T17:51:37Z

Description

This pull request extends upon #1156 to support both spatial and spectral subsets (including in the same dropdown, although we don't have any uses for that, yet). Most of this is just a generalization of logic, with a new Mixin for SpatialSubsetSelectMixin.

For the aperture photometry plugin, the dropdowns now look like:

Although not actually in use, a mixed dropdown would look like:

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 change log needed? If yes, is it added to CHANGES.rst?
Is a milestone set? Milestone is only currently required for PRs related to Imviz MVP.
After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

jdaviz/core/template_mixin.py

jdaviz/configs/imviz/plugins/aper_phot_simple/aper_phot_simple.py

see spacetelescope#1175 (comment)

pllim · 2022-03-18T00:33:00Z

jdaviz/core/template_mixin.py

-                                                subset_type="spectral").get(self.selected)
+        subset_type = self.selected_item['type']
+        for viewer_id in self.app.get_viewer_ids():
+            match = self.app.get_subsets_from_viewer(viewer_id,


Okay, I remember now. I did what I did to aperture photometry because of this:

Imviz: Aperture photometry plugin with viewer-agnostic data/subset dropdown menus #1096

see spacetelescope#1175 (comment)

codecov · 2022-03-23T20:17:12Z

Codecov Report

Merging #1175 (fe9fcc2) into main (752a0fb) will increase coverage by 0.39%.
The diff coverage is 89.89%.

@@            Coverage Diff             @@
##             main    #1175      +/-   ##
==========================================
+ Coverage   77.44%   77.83%   +0.39%     
==========================================
  Files          90       90              
  Lines        7079     7147      +68     
==========================================
+ Hits         5482     5563      +81     
+ Misses       1597     1584      -13

Impacted Files	Coverage Δ
jdaviz/core/template_mixin.py	`92.93% <88.09%> (-0.25%)`	⬇️
...igs/default/plugins/model_fitting/model_fitting.py	`30.63% <100.00%> (-0.66%)`	⬇️
...imviz/plugins/aper_phot_simple/aper_phot_simple.py	`94.82% <100.00%> (+0.02%)`	⬆️
...igs/specviz/plugins/line_analysis/line_analysis.py	`79.90% <100.00%> (ø)`
...nfigs/default/plugins/plot_options/plot_options.py	`93.87% <0.00%> (-2.68%)`	⬇️
jdaviz/core/marks.py	`78.57% <0.00%> (+0.17%)`	⬆️
jdaviz/configs/specviz/plugins/viewers.py	`76.20% <0.00%> (+7.88%)`	⬆️

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 752a0fb...fe9fcc2. Read the comment docs.

jdaviz/configs/imviz/plugins/aper_phot_simple/aper_phot_simple.py

jdaviz/configs/default/plugins/model_fitting/model_fitting.py

jdaviz/configs/imviz/plugins/aper_phot_simple/aper_phot_simple.py

jdaviz/configs/imviz/plugins/aper_phot_simple/aper_phot_simple.vue

jdaviz/core/template_mixin.py

pllim · 2022-03-24T13:42:30Z

jdaviz/core/template_mixin.py

+        if subset.label not in self.labels:
+            # NOTE: this logic will need to be revisited if generic renaming of subsets is added
+            # see https://github.com/spacetelescope/jdaviz/pull/1175#discussion_r829372470
+            if subset.label.startswith('Subset'):


xref glue-viz/glue-astronomy#30

pllim · 2022-03-24T13:43:24Z

jdaviz/core/template_mixin.py

@@ -230,32 +302,50 @@ def _update_has_subregions(self):
    def labels(self):
        return [s['label'] for s in self.items if 'label' in s.keys()]

+    @cached_property


Do we need a way to invalidate cache if we start to allow people to rename stuff?

Yes, I left a note in line ~273 that we would need to catch those events, refresh the items list, and clear the cache (the ability to clear cache already exists, we just need to trigger it in occasions that are necessary).

pllim · 2022-03-24T14:38:46Z

jdaviz/core/template_mixin.py

+        # that.  For imviz, this will mean we won't be able to loop through each of the viewers,
+        # but the original viewer should have access to all the subsets.
+        for viewer_ref in self.viewer_refs:
+            match = self.app.get_subsets_from_viewer(viewer_ref,


Will this break for Imviz, if say, Subset 1 is "hidden" (or not shown) in imviz-0 but it is shown in imviz-1 and people are working on the latter? I remember this is a problem for get_data_from_viewer and that is why I had to do #1096

Aperture photometry still uses your logic to actually extract the object (although in the future I think it would be nice to migrate to use the logic in the new component), and the dropdown is populated by messages, not looping over layers. If you can find a reproducible case that shows an issue, let me know (although I'd argue that we should fix get_data_from_viewer if it is failing to show items outside viewer limits or something like that... but we definitely want to avoid re-introducing broken behavior in this PR).

see spacetelescope#1175 (comment)

* so that initializing a plugin after subset creation will populate the original choices and then begin listening to messages

* aperture photometry tests used to handle gracefully ignoring invalid subset names, whereas the component now raises an error * bg_subset_items is now a dictionary, the old list can be accessed with bg_subset.labels

* component will consider all viewers unless viewer_refs is passed * spectral subset mixin passes spectrum-viewer by default * will ignore programmatically created imviz viewers which are not assigned a reference, but the subsets should be available from the original viewer.

Co-authored-by: P. L. Lim <[email protected]>

pllim

I only tested the plugins touched here, and the spectral ones only on Cubeviz. Basic usage on example notebooks seem to work fine. I didn't try very hard to break them. I think this is good to go in. We can always fine tune it as needed in follow-up PRs. Very nice UI/UX improvement for spatial subsets. Thanks!

jdaviz/core/template_mixin.py

duytnguyendtn

Wow, that was a lot of code; kudos on ripping all that out! I tested the plugins across the viz tools and everything seems to be intact! I have a few nitpicks about a few exception wordings, but overall looks great! Happy to approve once we resolve those.

I'll also note I confirmed I can still enter a manual value to the background in the aperture photometry section (as per #1175 (comment))

jdaviz/core/template_mixin.py

Co-authored-by: Duy Tuong Nguyen <[email protected]>

duytnguyendtn

Thanks for applying the suggestions! I'll merge when all the tests pass

kecnry · 2022-03-30T13:10:53Z

jdaviz/core/template_mixin.py

+    @property
+    def viewers(self):
+        return [self.app.get_viewer(ref) for ref in self.viewer_refs]


this is likely the cause of #1217 - when _subset_to_dict loops through the viewers by ref in imviz, only the original viewer has a reference. The subset still exists in the original viewer, so it is able to get the label and type, but the color must not be set when the message is received.

kecnry · 2022-03-30T13:11:00Z

jdaviz/core/template_mixin.py

+        # NOTE: we use reference names here instead of IDs since get_subsets_from_viewer requires
+        # that.  For imviz, this will mean we won't be able to loop through each of the viewers,
+        # but the original viewer should have access to all the subsets.
+        for viewer_ref in self.viewer_refs:


will also need to be updated when fixing #1217

github-actions bot added cubeviz imviz specviz labels Mar 17, 2022

kecnry added this to the 2.4 milestone Mar 17, 2022

kecnry force-pushed the subset-select-generalize branch 4 times, most recently from c70540c to 1f31e99 Compare March 17, 2022 18:16

kecnry commented Mar 17, 2022

View reviewed changes

jdaviz/core/template_mixin.py Outdated Show resolved Hide resolved

jdaviz/core/template_mixin.py Outdated Show resolved Hide resolved

kecnry force-pushed the subset-select-generalize branch from 1f31e99 to 76d0ba4 Compare March 17, 2022 18:19

pllim reviewed Mar 17, 2022

View reviewed changes

jdaviz/configs/imviz/plugins/aper_phot_simple/aper_phot_simple.py Show resolved Hide resolved

pllim reviewed Mar 17, 2022

View reviewed changes

jdaviz/configs/imviz/plugins/aper_phot_simple/aper_phot_simple.py Show resolved Hide resolved

kecnry added a commit to kecnry/jdaviz that referenced this pull request Mar 17, 2022

require subset to start with "Subset" to be a valid option

55fbf93

see spacetelescope#1175 (comment)

pllim reviewed Mar 18, 2022

View reviewed changes

kecnry added a commit to kecnry/jdaviz that referenced this pull request Mar 23, 2022

require subset to start with "Subset" to be a valid option

b087307

see spacetelescope#1175 (comment)

kecnry force-pushed the subset-select-generalize branch 3 times, most recently from 97d459a to 2b9995a Compare March 23, 2022 19:40

kecnry commented Mar 23, 2022

View reviewed changes

jdaviz/configs/imviz/plugins/aper_phot_simple/aper_phot_simple.py Show resolved Hide resolved

kecnry marked this pull request as ready for review March 23, 2022 20:28

kecnry requested review from duytnguyendtn, rosteen, javerbukh, ibusko and ojustino as code owners March 23, 2022 20:28

kecnry requested a review from pllim March 23, 2022 20:28

pllim reviewed Mar 23, 2022

View reviewed changes

pllim reviewed Mar 24, 2022

View reviewed changes

kecnry closed this Mar 24, 2022

kecnry reopened this Mar 24, 2022

kecnry added a commit to kecnry/jdaviz that referenced this pull request Mar 24, 2022

require subset to start with "Subset" to be a valid option

1d20a6b

see spacetelescope#1175 (comment)

kecnry force-pushed the subset-select-generalize branch 2 times, most recently from 9164880 to 84062a7 Compare March 24, 2022 20:26

kecnry added a commit to kecnry/jdaviz that referenced this pull request Mar 28, 2022

require subset to start with "Subset" to be a valid option

ba64ff3

see spacetelescope#1175 (comment)

kecnry force-pushed the subset-select-generalize branch from 84062a7 to feadd75 Compare March 28, 2022 12:42

kecnry added the Ready for final review label Mar 28, 2022

kecnry and others added 9 commits March 28, 2022 11:40

generalize subset select component to handle spatial and spectral

5f8cc34

require subset to start with "Subset" to be a valid option

3efe5ee

see spacetelescope#1175 (comment)

support initializing subset component AFTER subset creation

24fcba0

* so that initializing a plugin after subset creation will populate the original choices and then begin listening to messages

attempt to update aperture phot tests

e577ee7

* aperture photometry tests used to handle gracefully ignoring invalid subset names, whereas the component now raises an error * bg_subset_items is now a dictionary, the old list can be accessed with bg_subset.labels

smarter fallback when removed item was selected

73b0c9b

clarify reference vs id comment

e8acab8

Apply suggestions from code review

796ce11

Co-authored-by: P. L. Lim <[email protected]>

smarter default behavior when deleting selected entry

2610402

kecnry force-pushed the subset-select-generalize branch from feadd75 to 2610402 Compare March 28, 2022 15:40

pllim approved these changes Mar 28, 2022

View reviewed changes

duytnguyendtn reviewed Mar 28, 2022

View reviewed changes

jdaviz/core/template_mixin.py Show resolved Hide resolved

duytnguyendtn suggested changes Mar 28, 2022

View reviewed changes

jdaviz/core/template_mixin.py Outdated Show resolved Hide resolved

jdaviz/core/template_mixin.py Outdated Show resolved Hide resolved

Apply suggestions from code review

fe9fcc2

Co-authored-by: Duy Tuong Nguyen <[email protected]>

duytnguyendtn approved these changes Mar 28, 2022

View reviewed changes

duytnguyendtn merged commit 771f2bb into spacetelescope:main Mar 28, 2022

kecnry deleted the subset-select-generalize branch March 28, 2022 19:38

kecnry commented Mar 30, 2022

View reviewed changes

kecnry mentioned this pull request Mar 31, 2022

re-usable dataset select component #1221

Merged

9 tasks

kecnry mentioned this pull request May 4, 2022

WIP: SubsetSelect support for renaming subsets #1291

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

generalize subset select component to handle spatial and spectral #1175

generalize subset select component to handle spatial and spectral #1175

kecnry commented Mar 17, 2022 •

edited by pllim

Loading

pllim Mar 18, 2022

codecov bot commented Mar 23, 2022 •

edited

Loading

pllim Mar 24, 2022

pllim Mar 24, 2022

kecnry Mar 24, 2022

pllim Mar 24, 2022

kecnry Mar 24, 2022

pllim left a comment

duytnguyendtn left a comment

duytnguyendtn left a comment

kecnry Mar 30, 2022

kecnry Mar 30, 2022

generalize subset select component to handle spatial and spectral #1175

generalize subset select component to handle spatial and spectral #1175

Conversation

kecnry commented Mar 17, 2022 • edited by pllim Loading

Description

Checklist for package maintainer(s)

Choose a reason for hiding this comment

codecov bot commented Mar 23, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pllim left a comment

Choose a reason for hiding this comment

duytnguyendtn left a comment

Choose a reason for hiding this comment

duytnguyendtn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kecnry commented Mar 17, 2022 •

edited by pllim

Loading

codecov bot commented Mar 23, 2022 •

edited

Loading