Plugin plots to viewers

[kecnry]

Description

This pull request migrates all plugin plots (plot options stretch histogram, imviz's line profile, imviz's aperture photometry radial profile) to use a glue-viewer object, with their own independent app instances, instead of a bqplot object. This then allows us to use our nested toolbar on these plots (note that the data are not linked to the main data-collection, so using subset tools probably doesn't make sense). This is left somewhat flexible so that we can share the app instance when it makes sense - possibly for the compass plugin in the future.

For the stretch histogram, this removes the need for downsampling, but also loses the logic for ensuring the vmin/vmax markers are visible (since the limit state is handled by the viewer directly and the user can override - whereas previously the limits were fixed by code in the plugin).

Note that the histogram implementation includes some hacky workarounds due to bugs upstream. I'll work on reporting those and we can create tech debt tickets to remove the workarounds. The home button on the histogram is also known to be broken - we can either live with that until there is an upstream fix, remove the home button for the histogram in this PR, or block merging this PR until there is an upstream fix.

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

[pllim]

Is this the one that segfaults with fast-histogram? Should I hold off on reviewing?

[kecnry]

Is this the one that segfaults with fast-histogram? Should I hold off on reviewing?

It is, but I am quite sure that is an upstream bug (that will need to be addressed before merging). If it doesn't segfault for you, then I think its ready for reviewing! And if it does - the code can still be reviewed.

We also need to make decisions on the other "bugs" in the histogram viewer and whether those block merge or not, given that we want to build on top of this very soon.

[pllim]

I also got the segfault like CI and Ricky, so I cannot run the code, but I left some general comments.

[pllim]

What is the glue-jupyter PR that would include this upstream?

[kecnry]

Nothing yet, I've asked if it is intentionally excluded and if it isn't I'll open a PR there.

[kecnry]

glue-viz/glue-jupyter#402

[kecnry]

We now have a workaround (with guardrails) for the segfault. This should now be ready for review - please read the description as well as in-line comments about known issues and current workarounds before testing!

[rosteen]

This is pretty cool, I like it more than I thought I would. That said, I've encountered two issues. The first is that in the histogram viewer, the stretch curve does not seem to update based on panning and zooming correctly:

Screen.Recording.2023-10-09.at.9.42.44.AM.mov

And the second is that nothing plots for me in the line profile plugin:

Screen.Recording.2023-10-09.at.9.50.29.AM.mov

I took a cursory read through the code changes and nothing egregious stood out to me, but I'll try to take a closer look before actually approving. I figured I'd bring these bugs up now though.

[kecnry]

The first is that in the histogram viewer, the stretch curve does not seem to update based on panning and zooming correctly

This is by-design (but can be debated if it's what we want). The stretch function is not in the same units as the y-axis of the histogram, and so is essentially tied to a separate, normalized, y-axis. I thought about only allowing horizontal pan/zooming to avoid any confusion here, but I think it is useful to be able to zoom vertically on the tail of the histogram as well.

And the second is that nothing plots for me in the line profile plugin

hmmm, I can't reproduce that 🤔 Does something show if you press the home button? Right now I don't reset zoom on a change to the requested pixel, but we easily could. The downside would be if the user intentionally zooms and wants to keep that zoom when changing between pixels, but maybe we should instead have an option to lock that as a follow-up effort? EDIT: the plots now reset by default, if we ever want to disable that, we can add a toggle to lock the limits when data changes.

[codecov]

Codecov Report

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

Files	Coverage Δ
jdaviz/components/toolbar_nested.py	85.14% <100.00%> (+0.14%)	⬆️
...lt/plugins/plot_options/tests/test_plot_options.py	100.00% <100.00%> (ø)
...s/imviz/plugins/line_profile_xy/line_profile_xy.py	97.11% <100.00%> (-0.11%)	⬇️
jdaviz/configs/imviz/tests/test_line_profile_xy.py	100.00% <100.00%> (ø)
...daviz/configs/imviz/tests/test_simple_aper_phot.py	100.00% <100.00%> (ø)
...imviz/plugins/aper_phot_simple/aper_phot_simple.py	91.07% <96.66%> (+0.30%)	⬆️
...nfigs/default/plugins/plot_options/plot_options.py	86.36% <90.90%> (+3.43%)	⬆️
jdaviz/core/template_mixin.py	91.24% <97.14%> (+0.23%)	⬆️

... and 1 file with indirect coverage changes

📢 Thoughts on this report? Let us know!.

[rosteen]

Does something show if you press the home button?

Nope, nothing.

👍 on the histogram, maybe demo that offline at the review today and get feedback? I don't think it's super important but could potentially be confusing.

[kecnry]

Bizarre, can you dig into the object itself and see if the arrays are populated as expected?

lp = imviz.plugins['Imviz Line Profiles (XY)']
plot = lp._obj.plot_across_x

data = plot.app.data_collection[0]
print(data.get_component('x').data, data.get_component('y').data)

line_layer = plot.layers['line']
print(line_layer.layer.data['x'], line_layer.layer.data['y'])

[kecnry]

on the histogram, maybe demo that offline at the review today and get feedback? I don't think it's super important but could potentially be confusing.

Some feedback from review: this probably makes sense as-is for now, but we might need to reconsider when adding support for custom/interactive splines. The orange vertical line for vmin/vmax also looks like its a bug, so a different color might help (that we can do when making them interactive as well).

[pllim]

LGTM now.

I did see this weird thing where I thought I saw the scatter plot points flashed real quick but then they disappeared from the plot, but then if I pop the same plot out, I see them in the pop-out only. But I cannot reproduce it, so maybe we can chuck it under Heisenbug.

[rosteen]

Works for me now, code looks fine. Let's get this in so we can iterate on it.

[kecnry]

Ok, please hold off on merging for a bit - I'll merge aperture photometry first since that should be an easier order for the rebase 🤞