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

stretch histogram: handle mixed state #2606

Merged
merged 2 commits into from
Dec 15, 2023

Conversation

kecnry
Copy link
Member

@kecnry kecnry commented Dec 11, 2023

Description

This pull request implements the "mixed state overlay" for the stretch histogram when ANY of the inputs (stretch function, vmin, vmax, color mode, color, colormap - and knots should be added either her or #2545, whichever is merged second) are mixed and unmixes ALL of them when clicking.

Screen.Recording.2023-12-11.at.4.51.46.PM.mov

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 kecnry force-pushed the stretch-hist-mixed branch from 9327b46 to d353080 Compare December 11, 2023 21:54
@kecnry kecnry added the 💤backport-v3.8.x on-merge: backport to v3.8.x label Dec 11, 2023
@kecnry kecnry added this to the 3.8.1 milestone Dec 11, 2023
@kecnry kecnry added the imviz label Dec 11, 2023
Comment on lines 738 to 769
if self.layer_multiselect:
data = self.layer.selected_obj[0][0].layer
elif len(self.layer.selected_obj):
data = self.layer.selected_obj[0].layer
else:
if not len(self.layer.selected_obj):
# skip further updates if no data are available:
return
if isinstance(self.layer.selected_obj[0], list):
# multiselect case (but we won't check multiselect since the selection can lag behind)
data = self.layer.selected_obj[0][0].layer
else:
data = self.layer.selected_obj[0].layer
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this just fixes a weird bug I ran into where changing from multi -> single select which then applies the first selection as default triggered a length error here since self.layer_multiselect was temporarily out-of-date with the selection. I can split into a separate PR or changelog entry if anyone feels necessary.

@kecnry kecnry marked this pull request as ready for review December 12, 2023 13:56
@rosteen
Copy link
Collaborator

rosteen commented Dec 12, 2023

Hmm, this doesn't seem to work for me in Cubeviz (see video below). I don't see the mixed state un-mixers, and I get an error traceback if I try to switch to layer B.

Screen.Recording.2023-12-12.at.11.40.39.AM.mov

Traceback:

IndexError                                Traceback (most recent call last)
File ~/opt/anaconda3/envs/viz_dev/lib/python3.11/site-packages/ipywidgets/widgets/widget.py:773, in Widget._handle_msg(self, msg)
    771         if 'buffer_paths' in data:
    772             _put_buffers(state, data['buffer_paths'], msg['buffers'])
--> 773         self.set_state(state)
    775 # Handle a state request.
    776 elif method == 'request_state':

File ~/opt/anaconda3/envs/viz_dev/lib/python3.11/site-packages/ipywidgets/widgets/widget.py:650, in Widget.set_state(self, sync_data)
    645         self._send(msg, buffers=echo_buffers)
    647 # The order of these context managers is important. Properties must
    648 # be locked when the hold_trait_notification context manager is
    649 # released and notifications are fired.
--> 650 with self._lock_property(**sync_data), self.hold_trait_notifications():
    651     for name in sync_data:
    652         if name in self.keys:

File ~/opt/anaconda3/envs/viz_dev/lib/python3.11/contextlib.py:144, in _GeneratorContextManager.__exit__(self, typ, value, traceback)
    142 if typ is None:
    143     try:
--> 144         next(self.gen)
    145     except StopIteration:
    146         return False

File ~/opt/anaconda3/envs/viz_dev/lib/python3.11/site-packages/traitlets/traitlets.py:1512, in HasTraits.hold_trait_notifications(self)
   1510 for changes in cache.values():
   1511     for change in changes:
-> 1512         self.notify_change(change)

File ~/opt/anaconda3/envs/viz_dev/lib/python3.11/site-packages/ipywidgets/widgets/widget.py:701, in Widget.notify_change(self, change)
    698     if name in self.keys and self._should_send_property(name, getattr(self, name)):
    699         # Send new state to front-end
    700         self.send_state(key=name)
--> 701 super().notify_change(change)

File ~/opt/anaconda3/envs/viz_dev/lib/python3.11/site-packages/traitlets/traitlets.py:1527, in HasTraits.notify_change(self, change)
   1525 def notify_change(self, change: Bunch) -> None:
   1526     """Notify observers of a change event"""
-> 1527     return self._notify_observers(change)

File ~/opt/anaconda3/envs/viz_dev/lib/python3.11/site-packages/traitlets/traitlets.py:1570, in HasTraits._notify_observers(self, event)
   1567 elif isinstance(c, EventHandler) and c.name is not None:
   1568     c = getattr(self, c.name)
-> 1570 c(event)

File ~/projects/jdaviz/jdaviz/core/template_mixin.py:273, in skip_if_no_updates_since_last_active.<locals>.decorator.<locals>.wrapper(self, msg)
    271 if meth.__name__ not in self._methods_skip_since_last_active:
    272     self._methods_skip_since_last_active.append(meth.__name__)
--> 273 ret_ = meth(self, msg)
    274 if ret_ is False:
    275     self._methods_skip_since_last_active.remove(meth.__name__)

File ~/projects/jdaviz/jdaviz/configs/default/plugins/plot_options/plot_options.py:758, in PlotOptions._update_stretch_histogram(self, msg)
    755     return
    756 if isinstance(self.layer.selected_obj[0], list):
    757     # multiselect case (but we won't check multiselect since the selection can lag behind)
--> 758     data = self.layer.selected_obj[0][0].layer
    759 else:
    760     data = self.layer.selected_obj[0].layer

IndexError: list index out of range

@kecnry
Copy link
Member Author

kecnry commented Dec 13, 2023

I don't see the mixed state un-mixers

They should only show when the selected layer appears in multiple of the selected viewers AND have mixed state.

and I get an error traceback if I try to switch to layer B.

Fixed in the latest commit - thanks for catching this!

Copy link

codecov bot commented Dec 13, 2023

Codecov Report

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

Comparison is base (6a2411c) 91.55% compared to head (4696669) 91.53%.
Report is 7 commits behind head on main.

❗ Current head 4696669 differs from pull request most recent head 1c13b97. Consider uploading reports for the commit 1c13b97 to get more accurate results

Files Patch % Lines
...nfigs/default/plugins/plot_options/plot_options.py 73.68% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2606      +/-   ##
==========================================
- Coverage   91.55%   91.53%   -0.02%     
==========================================
  Files         161      161              
  Lines       19932    19847      -85     
==========================================
- Hits        18248    18167      -81     
+ Misses       1684     1680       -4     

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

Copy link
Collaborator

@rosteen rosteen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me now, approved. I did run into a case where the histogram viewer wasn't resizing when expanding the plugin tray, but I don't think it was caused by this and I haven't been able to reproduce it. I'll keep an eye out to see if it happens again.

Copy link
Contributor

@haticekaratay haticekaratay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested in Imviz! Looks good to me!

* single layer but with multiple viewers is supported by the stretch histogram, but could have different values for the curve, and so need to indicate that the state in mixed and have the overlay to unmix.
@kecnry kecnry force-pushed the stretch-hist-mixed branch from 4696669 to c634cbf Compare December 15, 2023 15:06
@kecnry
Copy link
Member Author

kecnry commented Dec 15, 2023

currently updating to also handle stretch params (knots) now that #2545 got merged first.

@kecnry kecnry marked this pull request as draft December 15, 2023 15:09
@kecnry kecnry force-pushed the stretch-hist-mixed branch 2 times, most recently from ac61f1e to 1c13b97 Compare December 15, 2023 17:30
@kecnry kecnry marked this pull request as ready for review December 15, 2023 17:34
@kecnry kecnry merged commit 71a943b into spacetelescope:main Dec 15, 2023
12 of 13 checks passed

This comment was marked as resolved.

@kecnry kecnry deleted the stretch-hist-mixed branch December 15, 2023 17:44
kecnry added a commit to kecnry/jdaviz that referenced this pull request Dec 15, 2023
* stretch histogram handle mixed state
* single layer but with multiple viewers is supported by the stretch histogram, but could have different values for the curve, and so need to indicate that the state in mixed and have the overlay to unmix.
* fix traceback in cubeviz when switching layers in multi viewer select
kecnry added a commit that referenced this pull request Dec 15, 2023
* stretch histogram handle mixed state
* single layer but with multiple viewers is supported by the stretch histogram, but could have different values for the curve, and so need to indicate that the state in mixed and have the overlay to unmix.
* fix traceback in cubeviz when switching layers in multi viewer select
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants