Spatial-Spectral highlighting #1528

kecnry · 2022-07-28T21:02:16Z

Description

This pull request implements spectral subset highlighting of automatically-collapsed spectra of spatial subsets in the profile viewer of cubeviz. It does so by a new custom bqplot mark which "shadows" the position of the mark corresponding to the spatial-spectrum, but the nan-mask and styling of the mark corresponding to the spectral-subset on the full spectrum. Everytime a new subset layer is added, a mark is created for each combination of the corresponding subset mark and all marks of the other (spatial vs spectral) type. Everytime a subset is removed, the marks are pruned to remove any where one of the referenced marks no longer exists. Pure hacky dark magic at its ugliest/finest.... and because of that this should be tested thoroughly.

Screen.Recording.2022-07-28.at.4.56.32.PM.mov

Screen.Recording.2022-07-28.at.4.58.06.PM.mov

Fixes #1179

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

kecnry · 2022-07-28T21:08:14Z

jdaviz/core/marks.py

+    def _get_id(self, mark):
+        return getattr(mark, '_model_id', None)
+
    def _setup_shadowing(self, shadowing, sync_traits=[], other_traits=[]):
-        self._shadowing = shadowing
-        self._sync_traits = sync_traits
+        """
+        sync_traits: traits to set now, and mirror any changes to shadowing in the future
+        other_trait: traits to set now, but not mirror in the future
+        """
+        if not hasattr(self, '_shadowing'):
+            self._shadowing = {}
+            self._sync_traits = {}
+        shadowing_id = self._get_id(shadowing)
+        self._shadowing[shadowing_id] = shadowing
+        self._sync_traits[shadowing_id] = sync_traits

        # sync initial values
        for attr in sync_traits + other_traits:
-            self._on_shadowing_changed({'name': attr, 'new': getattr(shadowing, attr)})
+            self._on_shadowing_changed({'name': attr,
+                                        'new': getattr(shadowing, attr),
+                                        'owner': shadowing})

        # subscribe to future changes
        shadowing.observe(self._on_shadowing_changed)

    def _on_shadowing_changed(self, change):
-        if change['name'] in self._sync_traits:
+        if change['name'] in self._sync_traits.get(self._get_id(change.get('owner')), []):
            setattr(self, change['name'], change['new'])
        return


this all extends the existing ShadowMixin to be able to shadow multiple marks simultaneously, the API remains the same, so should not affect other marks that make use of this, but we should thoroughly test those (line analysis continuum marks, slice indicator labels, etc)

codecov · 2022-07-29T12:40:07Z

Codecov Report

Merging #1528 (bab566a) into main (6060929) will increase coverage by 0.06%.
The diff coverage is 94.44%.

@@            Coverage Diff             @@
##             main    #1528      +/-   ##
==========================================
+ Coverage   85.64%   85.70%   +0.06%     
==========================================
  Files          94       94              
  Lines        9216     9271      +55     
==========================================
+ Hits         7893     7946      +53     
- Misses       1323     1325       +2

Impacted Files	Coverage Δ
jdaviz/configs/default/plugins/viewers.py	`95.04% <83.33%> (-0.74%)`	⬇️
jdaviz/configs/cubeviz/plugins/viewers.py	`89.20% <89.28%> (-0.08%)`	⬇️
jdaviz/configs/cubeviz/helper.py	`96.07% <100.00%> (ø)`
jdaviz/configs/specviz/plugins/viewers.py	`81.34% <100.00%> (-0.15%)`	⬇️
jdaviz/core/helpers.py	`26.08% <100.00%> (+0.53%)`	⬆️
jdaviz/core/marks.py	`90.52% <100.00%> (+0.77%)`	⬆️
jdaviz/core/tools.py	`75.44% <0.00%> (+1.19%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

kecnry · 2022-07-29T14:06:45Z

oooo good catch, we definitely need to fix that asap - but that doesn't go through any of the logic changed in this PR and I can confirm its also present on main. I'm guessing that was introduced by the recent linking changes?

To reproduce:

example cubeviz instance
~~create spatial subset~~ (not necessary to reproduce)
create spectral subset
model fitting: select subsets, fit a linear model

Edit: filed as #1530, fixed in #1531, will rebase once that is merged into main.

rosteen · 2022-07-29T14:07:33Z

Oh dear. I was thinking to myself "I'll check that this isn't on main...omg I hope it isn't in main" and hadn't checked yet.

javerbukh · 2022-07-29T21:47:11Z

#1531 has been merged so once this is rebased I can review, but looks good so far!

kecnry · 2022-07-29T22:48:37Z

Rebased and confirmed that on my end fitting a model to a spatial+spectral subset works as expected.

javerbukh

Code is code, nice work! 😄

javerbukh · 2022-08-01T13:29:14Z

jdaviz/configs/cubeviz/plugins/viewers.py

+        # use Lines/LinesGL (so an isinstance check won't be sufficient here)
+        layer_marks = [m for m in self.figure.marks if m.__class__.__name__ in ['Lines', 'LinesGL']]
+        # and now we'll assume that the marks are in the same order as the layers, this should
+        # be the case as long as the order isn't manually resorted


Hopefully we remember this when/if we introduce layer reordering.

javerbukh · 2022-08-01T13:31:35Z

@kecnry I just tried the spectrum at spaxel tool in this branch and noticed new subsets do not visually appear when you hold the Option key but it seems like they show up in the layers indicator on the viewer. Can you reproduce the same bug?

EDIT: User error, please ignore 😅 Just needed a clean conda environment.

PatrickOgle

Works as advertised.

pllim

I don't grok everything here. In a nutshell, looks good. Just some minor comments.

Also a shame the branch isn't called what_we_do_in_the_shadows. 😉

pllim · 2022-08-16T14:10:48Z

jdaviz/configs/cubeviz/plugins/viewers.py

+    def _get_spatial_subset_layers(self):
+        return [ls for ls in self.state.layers
+                if isinstance(getattr(ls.layer, 'subset_state', None), RoiSubsetState)]
+
+    def _get_spectral_subset_layers(self):
+        return [ls for ls in self.state.layers
+                if isinstance(getattr(ls.layer, 'subset_state', None), RangeSubsetState)]


Can't we combine this into one and swap the class to be compared depending on a keyword?

pllim · 2022-08-16T14:11:30Z

jdaviz/configs/cubeviz/plugins/viewers.py

+        layers_list = list(self.state.layers)
+        # here we'll assume that all custom marks are subclasses of Lines/GL but don't directly
+        # use Lines/LinesGL (so an isinstance check won't be sufficient here)
+        layer_marks = [m for m in self.figure.marks if m.__class__.__name__ in ['Lines', 'LinesGL']]


Didn't @duytnguyendtn write basically the same loop just recently somewhere else? Looks so familiar.

hmmm, good point. If that's already in main, I could try to replace both of them with a viewer.custom_marks property or something 🤔

Maybe I am thinking about #1550 that has been merged.

I created a viewer.native_marks and viewer.custom_marks... not really intended for public use, but could be useful for internal use. Not sure how everyone feels about those names though or if its worth the extra property on all viewers?

Not sure that I have a strong opinion here. Though I guess my immediate expectation would be more of a nested structure, like viewer.marks containing the native marks and then you could get the custom via viewer.marks.custom. But my opinion isn't strong

jdaviz/configs/cubeviz/plugins/viewers.py

pllim · 2022-08-16T14:14:35Z

jdaviz/configs/cubeviz/plugins/viewers.py

+            # need to add marks for every intersection between THIS spectral subset and ALL spatial
+            spatial_marks = self._get_marks_for_layers(self._get_spatial_subset_layers())
+            for spatial_mark in spatial_marks:
+                new_marks += [ShadowSpatialSpectral(spatial_mark, this_mark)]


Why the coverage say this is not covered?

ah, because the existing test in test_subsets/test_region_spectral_spatial only does spectral then spatial. I'll add a case for the other order to make sure this line gets covered (and test for the presence of the marks).

Edit: added the test, let's hope it now says this line is covered 🤞

* shadows the spectrum of a spatial subset but the mask and styling of a spectral subset

pllim

I didn't try to use it but the code LGTM. Thanks!

Unless @duytnguyendtn has opinions about the new method names, I think this is good to merge.

github-actions bot added cubeviz specviz labels Jul 28, 2022

kecnry added this to the 2.9 milestone Jul 28, 2022

kecnry force-pushed the spatial-spectral-intersection branch from 86182da to 0c734a6 Compare July 28, 2022 21:06

kecnry commented Jul 28, 2022

View reviewed changes

kecnry force-pushed the spatial-spectral-intersection branch 3 times, most recently from ddee9f7 to 23d6d0e Compare July 29, 2022 12:30

kecnry marked this pull request as ready for review July 29, 2022 12:43

kecnry requested review from duytnguyendtn, rosteen, javerbukh, ojustino and pllim as code owners July 29, 2022 12:43

kecnry added bug Something isn't working and removed specviz labels Jul 29, 2022

This comment was marked as resolved.

Sign in to view

kecnry force-pushed the spatial-spectral-intersection branch from 23d6d0e to 3719753 Compare July 29, 2022 22:48

javerbukh approved these changes Aug 1, 2022

View reviewed changes

kecnry added the Ready for final review label Aug 1, 2022

javerbukh reviewed Aug 1, 2022

View reviewed changes

kecnry force-pushed the spatial-spectral-intersection branch 2 times, most recently from 66015e6 to b5e7e22 Compare August 9, 2022 12:16

kecnry force-pushed the spatial-spectral-intersection branch from b5e7e22 to 6336eea Compare August 9, 2022 19:51

PatrickOgle approved these changes Aug 16, 2022

View reviewed changes

pllim reviewed Aug 16, 2022

View reviewed changes

kecnry added 6 commits August 16, 2022 11:27

extend ShadowMixin to shadow multiple marks simultaneously

3d02783

implement ShadowSpatialSpectral mark

c79a226

* shadows the spectrum of a spatial subset but the mask and styling of a spectral subset

automatically create/delete ShadowSpatialSpectral marks

8ce6581

changelog entry

bfc8c2b

generalize code with viewer property to access custom marks

c023766

improve test coverage for spatial-spectral marks

f6808b1

kecnry force-pushed the spatial-spectral-intersection branch from 6acbf40 to f6808b1 Compare August 16, 2022 15:28

add docstring

bab566a

pllim approved these changes Aug 16, 2022

View reviewed changes

kecnry merged commit d92e8aa into spacetelescope:main Aug 16, 2022

kecnry deleted the spatial-spectral-intersection branch August 16, 2022 16:16

kecnry mentioned this pull request Nov 18, 2022

FIX: shadowing/subsets after adding plugin data #1856

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Spatial-Spectral highlighting #1528

Spatial-Spectral highlighting #1528

kecnry commented Jul 28, 2022 •

edited

Loading

kecnry Jul 28, 2022

codecov bot commented Jul 29, 2022 •

edited

Loading

This comment was marked as resolved.

kecnry commented Jul 29, 2022 •

edited

Loading

rosteen commented Jul 29, 2022

javerbukh commented Jul 29, 2022

kecnry commented Jul 29, 2022

javerbukh left a comment

javerbukh Aug 1, 2022

javerbukh commented Aug 1, 2022 •

edited

Loading

PatrickOgle left a comment

pllim left a comment

pllim Aug 16, 2022

pllim Aug 16, 2022

kecnry Aug 16, 2022

pllim Aug 16, 2022

kecnry Aug 16, 2022

duytnguyendtn Aug 16, 2022

pllim Aug 16, 2022

kecnry Aug 16, 2022 •

edited

Loading

pllim left a comment

Spatial-Spectral highlighting #1528

Spatial-Spectral highlighting #1528

Conversation

kecnry commented Jul 28, 2022 • edited Loading

Description

Checklist for package maintainer(s)

Choose a reason for hiding this comment

codecov bot commented Jul 29, 2022 • edited Loading

Codecov Report

This comment was marked as resolved.

kecnry commented Jul 29, 2022 • edited Loading

rosteen commented Jul 29, 2022

javerbukh commented Jul 29, 2022

kecnry commented Jul 29, 2022

javerbukh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javerbukh commented Aug 1, 2022 • edited Loading

PatrickOgle left a comment

Choose a reason for hiding this comment

pllim left a 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

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

kecnry Aug 16, 2022 • edited Loading

Choose a reason for hiding this comment

pllim left a comment

Choose a reason for hiding this comment

kecnry commented Jul 28, 2022 •

edited

Loading

codecov bot commented Jul 29, 2022 •

edited

Loading

kecnry commented Jul 29, 2022 •

edited

Loading

javerbukh commented Aug 1, 2022 •

edited

Loading

kecnry Aug 16, 2022 •

edited

Loading