Improve live-collapsed spectrum contrast in cubeviz

[bmorris3]

Description

Live-collapsed spectra from spatial subsets in Cubeviz are quite low contrast when WebGL is used:

This PR increases the opacity when spatial subsets are drawn in the spectrum viewer if WebGL is being used to draw the spectrum layer, and the layer label is consistent with a subset:

I haven't added a test because I'm not sure where/when there updates could be made to the glue-jupyter setting which determines if GL is available.

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]

is there a reason this needs to loop over all layers/marks? Would acting directly only on layer_state (and the matching mark(s)) sent to this method get overwritten after-the-fact?

[bmorris3]

I'm not 100% sure why that doesn't work. If I try like you suggest:

            # change opacity for live-collapsed spectra from spatial subsets in Cubeviz:
            mark = self._get_marks_for_layers([layer_state.layer])

            # if using WebGL and this is a subset:
            if isinstance(mark, LinesGL) and layer_state.layer.label.startswith("Subset"):
                mark.set_trait('opacities', [0.8])

it seems like the alpha of the circular subset in the flux-viewer is synced to the alpha of the profile that represents that subset. So that approach seems to always change the alpha of both the circle and the profile.

[kecnry]

I think they'll be linked anyways via the plot options (unless we want to break that link specifically for opacities... we definitely want the colors to remain linked) 🤔

[bmorris3]

This (in retrospect, obvious) improvement borrows from the code in the other if-block in this method: 20ad4d1. It now loops over only the spatial subsets.

[codecov]

Codecov Report

Patch coverage: 100.00% and project coverage change: +90.61% 🎉

Comparison is base (bbcdcc8) 0.00% compared to head (9d7527d) 90.61%.

❗ Current head 9d7527d differs from pull request most recent head 59b9977. Consider uploading reports for the commit 59b9977 to get more accurate results

Additional details and impacted files

@@            Coverage Diff            @@
##           main    #2387       +/-   ##
=========================================
+ Coverage      0   90.61%   +90.61%     
=========================================
  Files         0      159      +159     
  Lines         0    18183    +18183     
=========================================
+ Hits          0    16477    +16477     
- Misses        0     1706     +1706     

Files Changed	Coverage Δ
jdaviz/configs/cubeviz/plugins/viewers.py	93.00% <100.00%> (ø)

... and 158 files with indirect coverage changes

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

[kecnry]

I'm a bit torn on this. If we change this to edit the layer state instead, then plot options will pick up the value and make more sense, but it does tie the line opacity with the overlay in the image viewer (and therefore this PR would increase the opacity of the spatial subsets to 0.8 as well, which might not be ideal).

What you have here is nice because it doesn't affect the overlay, but then when you go in plot options its a little confusing because it shows opacity of 0.5, and "increasing" that to 0.51, for example, ends up decreasing the opacity (since it was actually 0.8 previously), and from then on the line and overlay are synced.

Suggested change

	if isinstance(mark, Lines):
	mark.set_trait('opacities', [0.8])
	if isinstance(mark, Lines):
	layer.alpha = 0.8

[rosteen]

I think this is a problem with the opacity slider, not with this PR. Given that the opacity slider is used in the context of having the spectrum viewer selected in plot options, it should really only update the opacity for the subset marks in the spectrum viewer, not the subset marks in all viewers for that subset. IMO anyway. It might need an upstream refactor to update viewer-specific marks rather than sharing the opacities between viewers for the subset layers.

[kecnry]

I think (but am not positive) that this is a limitation of how the state is stored in glue/glue-jupyter, and not sure there is much we can do short of a custom callback to override... but it is definitely worth checking that it isn't plot options that are syncing the states together.

[pllim]

FWIW, I am a bit 👎 on the route that would decouple this from Plot Options sync. I feel like that is going to come back and bite us.

[haticekaratay]

Thank you, @bmorris3, for assisting me offline in setting up my system. This allowed me to test the PR effectively and locally verify that the view is functioning as intended. I will leave the code review to others on the team.

[kecnry]

Even with plot options removed from the app, doing the following changes the opacity of both the spatial subset overlay and the spectral line, which I think demonstrates that the state in glue is inherently linked.

# open cubeviz, create spatial subset
viz.app.get_viewer('spectrum-viewer').layers[1].state.alpha = 1.0

It sounds to me like we have a few options going forward:

1. This PR as-is, initially but temporarily decoupling the spectrum line's opacity from its state. Once anything updates the state.alpha (through plot options or otherwise), they would then be re-coupled. @pllim gives a 👎 to this option, and I think I would as well.
2. Update this PR to set the default state.alpha instead, which would then increase the default for both the line and the overlay. Is there a value that would be a satisfactory default for a line sufficiently visible but the overlay not too opaque? We can also keep in mind that Model fitting table #2093 added a plugin for spectral extraction which would have a default opacity of 1 in the resulting spectra, so maybe keeping a distinction with the live-collapse lines would be a feature, not a bug.
3. Implement some sort of callback, perhaps in plot options, where the state.alpha is cut in half (for example) for the spatial subset overlay. This would then mean there would always be a fixed relationship between the spatial overlay and the live-collapsed spectrum, but would also prevent setting the overlay to ever have an opacity over 0.5 (in this example). The implementation might also be hacky and could result in flickering depending on where we can hook in the callback to update the marks.

[bmorris3]

I'm in favor of (3) like we proposed on our call yesterday. Drawing a spatial subset that's less transparent than our current default will make it difficult to assess if faint sources are actually in the subset.

[bmorris3]

In the latest commits, I've made the spectrum profile 50% more opaque than the spatial subset. Here's what it looks like:

I also found and fixed a bug from #2388 – spatial subsets can be selected in Plot Options, and if you select one in Cubeviz, an error is raised because the stretch histogram can't be updated from a subset. Because of this error, currently the opacity of spatial subsets cannot be updated on main.

[bmorris3]

And even after fixing the above bug - how is one supposed to change the opacity of a spatial subset from either the user API or the Plot Opts plugin? I tested both options I could think of (update the *_opacity* traitlets, click on the alpha slider in the plot options color picker), and neither works.

[kecnry]

I also found and fixed a bug from #2388 – spatial subsets can be selected in Plot Options, and if you select one in Cubeviz, an error is raised because the stretch histogram can't be updated from a subset. Because of this error, currently the opacity of spatial subsets cannot be updated on main.

I don't think this was introduced by #2388 - it might be worth checking if its in the current release and trying to include a patch just for this in a bugfix PR?

[kecnry]

how is one supposed to change the opacity of a spatial subset from either the user API or the Plot Opts plugin?

We currently only expose it for the line itself (need to select "spectrum-viewer"), but because the way the state is handled in glue, it affects all viewers 🙄

[pllim]

How hard is it for Subset display to have a different alpha for edges and the fill (not unlike footprints)? Is this something possible in glue-jupyter?

[pllim]

I asked because then you can set the subset edge alpha to totally opaque (and also the spectrum) and it would be no problem because that won't cover the data.

[pllim]

Okay nevermind... it is rendered as 2D mask array, so do separate out edges, we need edge detection. Not sure if that is worth it.

https://github.com/glue-viz/glue/blob/1a5c7676c025a1a025068b806f6f90ed53bba543/glue/viewers/image/state.py#L584C50-L584C50

Also... 😅

https://github.com/glue-viz/glue/blob/1a5c7676c025a1a025068b806f6f90ed53bba543/glue/viewers/image/state.py#L589-L590

# TODO: we can save memory by not showing subset multiple times for
# different image datasets since the footprint should be the same.

[kecnry]

After discussion, sounds like option 2 above isn't a good solution (there isn't a single opacity level that would work for both viewers).

Looking at those links @pllim... I wonder if we could ask for that global_sync thing to be extended to these subset layers? Then maybe we could decouple them and control them separately from jdaviz as @rosteen suggested?

If that isn't an option... I can try to hack at a proof-of-concept for a callback to keep the opacities coupled but at a fixed ratio and then we can decide whether to move forward in that direction or this PR as-is (or we can always review/merge this as-is and consider that for follow-up).

[kecnry]

The group decided to merge this as-is for now as an improvement over the current behavior and then hopefully improve in a more robust way with changes upstream down the road.

Might benefit from a rebase though to get #2393 from the diff 🤷 (but looks like GitHub isn't complaining about any conflicts, so probably doesn't matter).