Add masked subset support #2462

javerbukh · 2023-09-15T13:28:53Z

Description

This pull request is a continuation of #2421, which enables freezing of subsets. This converts them to a MultiMaskSubsetState which in this PR can be returned through app.get_subsets() and is visible in the Subset Plugin. If simplify_subset is set to True and there are MultiMaskSubsetState objects in the Composite subset, they will be ignored so that the Range objects can be combined into a SpectralRegion object.

To get the Freeze button to appear:

plugin = viz.app.get_tray_item_from_name('g-subset-plugin')
plugin.can_freeze = True

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.
Add support for MultiMaskSubsetState in get_subsets() and the subset plugin.

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

codecov · 2023-09-15T14:22:22Z

Codecov Report

Patch coverage is 87.03% of modified lines.

Files Changed	Coverage
jdaviz/utils.py	`76.92%`
jdaviz/app.py	`91.66%`
...igs/default/plugins/subset_plugin/subset_plugin.py	`100.00%`

📢 Thoughts on this report? Let us know!.

kecnry

Just a few small comments/suggestions, but this seems to work quite well and is a surprisingly-light weight solution, thanks!!

jdaviz/configs/default/plugins/subset_plugin/subset_plugin.vue

jdaviz/utils.py

jdaviz/configs/default/plugins/subset_plugin/subset_plugin.vue

kecnry · 2023-09-18T14:07:50Z

jdaviz/app.py

@@ -972,6 +972,8 @@ def get_subsets(self, subset_name=None, spectral_only=False,
                subset_region = self._get_range_subset_bounds(subset.subset_state,
                                                              simplify_spectral,
                                                              use_display_units)
+            elif isinstance(subset.subset_state, MultiMaskSubsetState):
+                subset_region = self._get_multi_mask_subset_definition(subset.subset_state)
            else:


What else might still fall under this else? Can it return any reasonable default in that case or raise an error instead? I just worry about future cases where nothing getting returned could result in unintentionally missing something.

Do you have a preference between reasonable default and raise error? I lean towards default but I can see the merits of either one.

I think default is probably fine for this case, even if it doesn't contain any information besides the class name or something, that is still more instructive than a missing entry.

rosteen · 2023-09-18T15:46:09Z

Looks pretty good, one question from testing: after freezing my composite subset to a mask, when I use get_subset to retrieve it the result is a list:

imviz.app.get_subsets()['Subset 1']
>>> [{'name': 'MultiMaskSubsetState',
    'glue_state': 'MultiMaskSubsetState',
    'region': 986526,
    'subset_state': <jdaviz.utils.MultiMaskSubsetState at 0x17ba5e6d0>}]

Is there any case where this list would have multiple entries? It seems like there isn't, in which case it would be better to return just the dictionary without the list wrapper around it.

rosteen · 2023-09-18T15:55:03Z

jdaviz/app.py

@@ -1068,6 +1071,12 @@ def _get_range_subset_bounds(self, subset_state,
                     "subset_state": subset_state}]
        return spec_region

+    def _get_multi_mask_subset_definition(self, subset_state):
+        return [{"name": subset_state.__class__.__name__,


Ah, you're explicitly wrapping the dict in a list here. What was the reason for this?

The reason for this is that MultiMask could be one region of many in a composite subset. To return all that information, we use a list of dictionaries which contain information on those subregions.

Ahh, I missed (or forgot) that get_subsets() always returns a list of dictionaries now, even the subset is a single region. As long as it's consistent, carry on nothing to see here 😄

pllim

How is this kind of subset supposed to work when "reparenting" becomes a thing? Or when Brett's rotation work gets merged?

Or is this never going to be enabled for Imviz?

javerbukh · 2023-09-18T16:27:25Z

@pllim Good question, this use case is more for LCViz so it will not be enabled for Imviz, at least not this iteration.

rosteen · 2023-09-18T18:59:53Z

@pllim Good question, this use case is more for LCViz so it will not be enabled for Imviz, at least not this iteration.

I did test this in Imviz and it worked, but I haven't thought through any implications/interactions due to the things @pllim brought up. I think adding this to the conversion through WCS links for reparenting should be easy, not sure about rotation. I think I'm comfortable merging given my current ongoing work.

kecnry

LGTM - feel free to add the change log and merge whenever ready!

fix event handling for choosing config from launcher filepath fallback when object from config-detection fails to load fix: support for notebook 7 (spacetelescope#2420) Notebook 7 requires a different way to detect if the app runs in a notebook context. Fix explanation in markdown cell. DOC: Footprints plugin API docs (spacetelescope#2426) * build footprints API docs * add code snippet in plugin docs * fix syntax in API docs --------- Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com> prevent overwriting user-API methods (spacetelescope#2425) fix default color for overlay when traitlet set by user * for the FIRST overlay, the user could have set the color traitlet before the internal overlay object is created (which occurs as soon as the plugin is first shown). In that case, we want to respect the user choice of color. similar fixes for any other traitlet when plugin inactive regression test change log entry fix API import of single region and overwriting existing entry * along with regression testing TST: Also pull in scikit-image dev to get rid of incompatibility with numpy Use new "true circle" tool from glue-jupyter (spacetelescope#2332) * Use truecircle tool from glue-viz/glue-jupyter#376 * MNT: Bump glue-jupyter minversion that can use this new tool * Add change log * Fix line too long * Fix change log verbiage Co-authored-by: Kyle Conroy <kyleconroy@gmail.com> --------- Co-authored-by: Kyle Conroy <kyleconroy@gmail.com> Enable multiselect in subset plugin for recentering (spacetelescope#2430) * Enable multiselect in subset plugin for recentering * Fix bug with default text * Remove unused import * Fix bug in aperature photometry * Address review comments * Remove commented out code * Add initial tests * Fix bug when exiting multiselect and switching subsets * Add documentation and update CHANGES file * Remove print statement * Add comment about recentering taking multiple iterations to docs * fix chip styling in subset dropdown * only set selected_has_subregions if exists * only show multiselect toggle in imviz * Apply suggestions from code review Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com> --------- Co-authored-by: Kyle Conroy <kyleconroy@gmail.com> Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com> Debug standalone build (spacetelescope#2441) * Log jdaviz output for debugging * Run workflow on debugging branch * See if collecting data from jupyter-client will fix this, otherwise might need to downgrade * Remove debugging stuff MNT: Bump actions/checkout to v4 Enable workflow_dispatch for standalone because it cannot be enabled outside of main apparently [ci skip] [rtd skip] EXP: Update workflow versions Fix Mosviz slit overlay (spacetelescope#2434) * Fix Mosviz slit overlay by using polygon instead of forcing a rectangle without angle info. Co-authored-by: Camilla Pacifici <camilla.pacifici@gmail.com> * Off by one error in predicted PR num --------- Co-authored-by: Camilla Pacifici <camilla.pacifici@gmail.com> API access (hidden) to disable cubeviz movie UI (spacetelescope#2440) * (hidden) API access to disable cubeviz movie UI * default movie_enabled based on new app-setting server_is_remote * test coverage TST: Ignore ASDF warning about gwcs-1.0.0 schema because gwcs dev and/or asdf-wcs-schemas 0.2.0 triggers the warning because the schema is no longer supported.

CHANGES.rst

Co-authored-by: Kyle Conroy <kyleconroy@gmail.com>

* Integrate MultiMaskSubsetState into get_subsets and subset_plugin --------- Co-authored-by: Kyle Conroy <kyleconroy@gmail.com>

github-actions bot added documentation Explanation of code and concepts cubeviz specviz mosviz embed Regarding issues with front-end embedding testing imviz specviz2d labels Sep 15, 2023

javerbukh force-pushed the add-masked-subset-support branch from 0856fcb to e5b3cfa Compare September 15, 2023 13:35

javerbukh marked this pull request as ready for review September 15, 2023 14:32

javerbukh requested review from rosteen, pllim, kecnry, bmorris3, haticekaratay and cshanahan1 as code owners September 15, 2023 14:32

kecnry reviewed Sep 18, 2023

View reviewed changes

This was referenced Sep 18, 2023

(disabled) ability to "freeze" subsets by converting to masks #2421

Closed

enable subset freezing spacetelescope/lcviz#45

Merged

rosteen reviewed Sep 18, 2023

View reviewed changes

pllim reviewed Sep 18, 2023

View reviewed changes

rosteen approved these changes Sep 18, 2023

View reviewed changes

kecnry approved these changes Sep 18, 2023

View reviewed changes

kecnry and others added 2 commits September 18, 2023 16:07

Integrate MultiMaskSubsetState into get_subsets and subset_plugin

578c22d

javerbukh force-pushed the add-masked-subset-support branch from 93698c5 to 578c22d Compare September 18, 2023 20:08

javerbukh added the skip-changelog-checks changelog bot directive label Sep 18, 2023

kecnry reviewed Sep 18, 2023

View reviewed changes

CHANGES.rst Outdated Show resolved Hide resolved

Update CHANGES.rst

1579210

Co-authored-by: Kyle Conroy <kyleconroy@gmail.com>

javerbukh removed the skip-changelog-checks changelog bot directive label Sep 18, 2023

javerbukh added this to the 3.7 milestone Sep 18, 2023

javerbukh merged commit d1f24ee into spacetelescope:main Sep 18, 2023
13 of 15 checks passed

javerbukh deleted the add-masked-subset-support branch September 19, 2023 00:19

bmorris3 pushed a commit to bmorris3/jdaviz that referenced this pull request Sep 22, 2023

Add masked subset support (spacetelescope#2462)

a7a5aec

* Integrate MultiMaskSubsetState into get_subsets and subset_plugin --------- Co-authored-by: Kyle Conroy <kyleconroy@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add masked subset support #2462

Add masked subset support #2462

javerbukh commented Sep 15, 2023 •

edited

Loading

codecov bot commented Sep 15, 2023 •

edited

Loading

kecnry left a comment

kecnry Sep 18, 2023

javerbukh Sep 18, 2023

kecnry Sep 18, 2023

rosteen commented Sep 18, 2023

rosteen Sep 18, 2023

javerbukh Sep 18, 2023

rosteen Sep 18, 2023

pllim left a comment

javerbukh commented Sep 18, 2023

rosteen commented Sep 18, 2023

kecnry left a comment

Add masked subset support #2462

Add masked subset support #2462

Conversation

javerbukh commented Sep 15, 2023 • edited Loading

Description

Change log entry

Checklist for package maintainer(s)

codecov bot commented Sep 15, 2023 • edited Loading

Codecov Report

kecnry left a comment

Choose a reason for hiding this comment

kecnry Sep 18, 2023

Choose a reason for hiding this comment

javerbukh Sep 18, 2023

Choose a reason for hiding this comment

kecnry Sep 18, 2023

Choose a reason for hiding this comment

rosteen commented Sep 18, 2023

rosteen Sep 18, 2023

Choose a reason for hiding this comment

javerbukh Sep 18, 2023

Choose a reason for hiding this comment

rosteen Sep 18, 2023

Choose a reason for hiding this comment

pllim left a comment

Choose a reason for hiding this comment

javerbukh commented Sep 18, 2023

rosteen commented Sep 18, 2023

kecnry left a comment

Choose a reason for hiding this comment

javerbukh commented Sep 15, 2023 •

edited

Loading

codecov bot commented Sep 15, 2023 •

edited

Loading