Plot options rewrite

[kecnry]

Description

This pull request implements custom jdaviz plot options that we can modify/style as well as an "advanced" mode (off by default) that allows for editing plot options across multiple viewers/layer simultaneously. In order to do this, the Select plugin components had to be updated to support toggling between single and multi-select, and a new Select component had to be created for generic (both subset and data) layers.

When in "advanced" mode, entries with "mixed-states" between the different entries will have an overlay that can be clicked to "unmix" the states:

This PR does not address all the styling changes requested for the plot options yet, but provides the framework for us to make those changes in follow-up efforts.

This supersedes the proof-of-concept in #1103.

In the default "simple" mode, the behavior is very similar to previously, except the plot options have been separated into sections and re-ordered to be more useful in jdaviz:

Screen.Recording.2022-05-25.at.10.17.51.AM.mov

Turning on "multiselect" mode now enables more advanced use-cases:

Screen.Recording.2022-05-25.at.10.18.48.AM.mov

including the ability to set stretch across multiple layers simultaneously:

Screen.Recording.2022-05-25.at.10.21.42.AM.mov

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

[codecov]

Codecov Report

Merging #1343 (0f32c8e) into main (410361e) will increase coverage by 0.15%.
The diff coverage is 90.23%.

@@            Coverage Diff             @@
##             main    #1343      +/-   ##
==========================================
+ Coverage   84.78%   84.93%   +0.15%     
==========================================
  Files          91       91              
  Lines        8023     8339     +316     
==========================================
+ Hits         6802     7083     +281     
- Misses       1221     1256      +35     

Impacted Files	Coverage Δ
jdaviz/app.py	92.75% <ø> (ø)
jdaviz/core/tools.py	72.00% <28.57%> (-3.00%)	⬇️
jdaviz/configs/specviz/plugins/viewers.py	81.10% <75.00%> (-1.97%)	⬇️
jdaviz/core/template_mixin.py	91.32% <88.34%> (-1.34%)	⬇️
...nfigs/default/plugins/plot_options/plot_options.py	100.00% <100.00%> (+2.63%)	⬆️
jdaviz/core/freezable_state.py	86.66% <100.00%> (+0.95%)	⬆️
jdaviz/configs/cubeviz/plugins/tools.py	84.80% <0.00%> (-0.24%)	⬇️

---

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 410361e...0f32c8e. Read the comment docs.

[Jenneh]

Looks good. I see more styling updates for the general plugin but that is outside the scope of this ticket.

[pllim]

Needs a rebase to fix change log -- FYI

[pllim]

SOLVED: I get this traceback if I create a second viewer in Imviz, load an image in the second viewer, and then select the second viewer in Plot Options.

``` .../ipywidgets/widgets/widget.py in _handle_msg(self, msg) 674 if 'buffer_paths' in data: 675 _put_buffers(state, data['buffer_paths'], msg['buffers']) --> 676 self.set_state(state) 677 678 # Handle a state request.

.../ipywidgets/widgets/widget.py in set_state(self, sync_data)
543 from_json = self.trait_metadata(name, 'from_json',
544 self._trait_from_json)
--> 545 self.set_trait(name, from_json(sync_data[name], self))
546
547 def send(self, content, buffers=None):

.../contextlib.py in exit(self, typ, value, traceback)
124 if typ is None:
125 try:
--> 126 next(self.gen)
127 except StopIteration:
128 return False

.../traitlets/traitlets.py in hold_trait_notifications(self)
1214 for changes in cache.values():
1215 for change in changes:
-> 1216 self.notify_change(change)
1217
1218 def _notify_trait(self, name, old_value, new_value):

.../ipywidgets/widgets/widget.py in notify_change(self, change)
604 # Send new state to front-end
605 self.send_state(key=name)
--> 606 super(Widget, self).notify_change(change)
607
608 def repr(self):

.../traitlets/traitlets.py in notify_change(self, change)
1227 def notify_change(self, change):
1228 """Notify observers of a change event"""
-> 1229 return self._notify_observers(change)
1230
1231 def _notify_observers(self, event):

.../traitlets/traitlets.py in _notify_observers(self, event)
1264 c = getattr(self, c.name)
1265
-> 1266 c(event)
1267
1268 def _add_notifiers(self, handler, name, type):

.../jdaviz/core/template_mixin.py in _on_viewer_changed(self, msg)
448 if new != old:
449 self._clear_cache()
--> 450 self._on_layers_changed()
451
452 @observe('filters')

.../jdaviz/core/template_mixin.py in _on_layers_changed(self, msg)
457 if not isinstance(viewer_names, list):
458 viewer_names = [viewer_names]
--> 459 viewers = [self.app.get_viewer(viewer) for viewer in viewer_names]
460
461 manual_items = [{'label': label} for label in self.manual_options]

.../jdaviz/core/template_mixin.py in (.0)
457 if not isinstance(viewer_names, list):
458 viewer_names = [viewer_names]
--> 459 viewers = [self.app.get_viewer(viewer) for viewer in viewer_names]
460
461 manual_items = [{'label': label} for label in self.manual_options]

.../jdaviz/app.py in get_viewer(self, viewer_reference)
439 The viewer class instance.
440 """
--> 441 return self._viewer_by_reference(viewer_reference)
442
443 def get_viewer_by_id(self, vid):

.../jdaviz/app.py in _viewer_by_reference(self, reference)
997 viewer_item = self._viewer_item_by_reference(reference)
998
--> 999 return self._viewer_store[viewer_item['id']]
1000
1001 def _viewer_item_by_reference(self, reference):

TypeError: 'NoneType' object is not subscriptable

</details>

[kecnry]

Rebased and fixed handling retrieving layers for imviz viewers without names.

I would argue that maybe we don't even need "Show axes" for Imviz. If you enable it and then disable it again, there is whitespace around the viewer that you cannot get rid of, reducing the display real estate.

It's easy to hide if that's what we want to do (maybe there are cases where you want to see the scale, but at least this information is available from toolbar when mousing over pixels). The whitespace is an issue upstream that also affects cubeviz/mosviz - we need to force a redraw when toggling the axes. If you resize the viewer panel, it will force it to adjust and fill the space appropriately. I'll bring this up for discussion and we can decide whether to keep or remove the toggle in imviz.

Addressed

[pllim]

For Imviz:

1. Show axis toggle should be removed, as discussed.
2. Layer should only show loaded data, not the Subsets and any loaded markers table. In the screenshot below, it should only show the first 2 entries. Selecting a subset or loaded markers table does not give me any useful options anyway.

Otherwise, it seems to work great for Imviz. And I think allowing people to set stretch etc for multiple viewers at once is a killer feature. Great work! 👏

[pllim]

In Cubeviz, it gets confusing to see FLUX listed twice when select all is done for everything. I understand that one of them is the spectrum but I am not sure if a new user would know that. I wonder if Select All should exclude the spectrum viewer...

p.s. When I am in multi-select mode, sometimes the options like stretch does not show even though I only have one thing selected in that mode. But if I change back to simple mode and select the same thing, it shows. I do not know the exact steps to reproduce this as I was clicking on things to look at other stuff.

[kecnry]

I think I've addressed most/all of the previous comments. I couldn't reproduce some of the tracebacks anymore, but am not sure the exact workflow to reproduce them, so please do let me know if you can still find ways to trigger them.

I think there's still one big open question for this PR (or possibly as a follow-up): how should options for subsets be categorized? Right now, options for subsets in the profile viewer show up under the line subsection (and are toggled with "show line"), whereas subsets in the image viewer are toggled with "show subset" but do NOT have any exposed options for color or opacity.

Changes to consider:

1. Create a new subsection for subsets so that subsets in image viewers can have their color/opacity controlled there. Do subset layers in spectral viewers get moved to this section and out of the line section (in which case there would be duplicated options like line width that would not be possible to group with data entries when in advanced mode)?

2. Lump all visibility toggles together ("show line", "show image", "show subset" -> "visible") except for "show contour" which must be separate. On one hand this might make the terminology less confusing, but it also adds confusion since contour must be separate.

This is all very hard to explain in text or screenshots, the best way to understand is to open cubeviz, create a spatial subset and a spectral subset, and then play with the plot options, especially in advanced mode.

[pllim]

I probably forgot some discussions... where did the contour line controls go?

[pllim]

Running the Imviz example notebook, I added "my_sky" markers and then removed them using viewer.remove_markers(marker_name='my_sky'). It is gone from Data dropdown but still in the Plot Options dropdown.

I have to stop here for the day, so this is by no means final review comment. I wonder if we should just get this in, and then open follow-up issues to avoid this dragging on and getting stale...

[camipacifici]

I agree with @pllim. Lets get this in and test with real use cases.

[javerbukh]

I agree with the other reviewers. This is a marked improvement over the previous plot options, even though it still needs a few iterations. I would suggest delaying the 2.7 release for another sprint to give time for any bug fixes after we merge this. Great work!!

P.S. maybe the dark mode fix could be at the top of that bug list? 😄

[pllim]

How about when Kyle is back next week, we make a list of follow-up issues and then merge this?

[pllim]

I am just gonna approve this with the agreement that this PR will have a list of follow-up issues linked to it as future work.