DQ support for Cubeviz #2817
DQ support for Cubeviz #2817
Conversation
github-actions
bot
added
cubeviz
imviz
plugin
bmorris3
force-pushed
the
cubeviz-dq
branch
3 times, most recently
from
3787611 to
95fa409
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2817 +/- ##
==========================================
+ Coverage 88.88% 88.94% +0.05%
==========================================
Files 111 111
Lines 16983 17101 +118
==========================================
+ Hits 15096 15211 +115
- Misses 1887 1890 +3 ☔ View full report in Codecov by Sentry. |
bmorris3
requested review from
rosteen,
javerbukh,
pllim,
haticekaratay,
cshanahan1 and
gibsongreen
as code owners
|
The data menu behavior is a little glitchy when adding another cube into the flux viewer (although maybe this is unrelated and just exposed by the new behavior here). Screen.Recording.2024-04-19.at.7.48.32.AM.movScreen.Recording.2024-04-19.at.7.49.26.AM.movI'm also seeing the "background" of the flux viewer being white now whereas it was black before (and the uncertainty viewer is still black). Is this intentional/expected? Unrelated (can also reproduce on main) but noticed in the video above: the moment map is showing rotated wrt the original cube 🤔 |
|
@kecnry I'm not positive, but it may be possible the new white background in |
|
The white background does seem to have been introduced in 1.20 - is that a concern with that upstream PR or is it now the preferred behavior? Why does it only happen in the flux viewer and not the uncertainty viewer? What is the expectation for support in other cube viewers (you did mention "it assumes that the flux-viewer is the only viewer that will contain the DQ layer" in the PR description, but the layer is available to load in the data menu)? This is the current behavior which is confusing (but fixing could probably also be a follow-up): Screen.Recording.2024-04-22.at.8.26.37.AM.movStill also needs:
|
RE @kecnry: The white background occurs in the I didn't opt-in to this feature for all layers everywhere, but we can (and I'd argue that we should). Dark-mode users might complain with default-white out-of-bounds regions though. 🕶 |
|
Here are the rendered DQ plugin docs in Imviz: 
|
| for layer in uncert_viewer.layers: | ||
| layer.composite._allow_bad_alpha = True | ||
| layer.force_update() |
There was a problem hiding this comment.
Screen.Recording.2024-04-23.at.1.46.03.PM.mov
I think this is a result of the background preferences, and it may be a null point. The relative opacity of the DQ layer does make the SCI data less visualizable compared to the previous commits and having the less desirable background. I'm unsure of the usability requirements when someone is focused on looking at DQ and if they would be okay toggling the layer on and off through the data menu.
There was a problem hiding this comment.
Thanks for these points. The DQ information is only useful if you can flip between science and DQ arrays, and see which science data may be affected by artifacts. I agree that it can be visually confusing to have both DQ and SCI open at the same, so we have the opacity slider and visibility in the data menu.
There was a problem hiding this comment.
Is the "darkening" of the image due to the DQ layer "fixed" by the upstream changes to glue? Or does that only affect outside the bounds of the DQ layer?
|
In 8118066, I added support for one DQ layer across multiple viewers: cubeviz-dq-multiviewer.mov |
|
Should changing the viewer update the UI to reflect the current state (rather than the latest state of the previously selected viewer)? Screen.Recording.2024-04-24.at.7.34.51.AM.movNow that multi-viewer support exists, this would also probably want to handle mixed state... which is going to get quite complicated for the flags unless the machinery from plot options can be adopted here. We could either live with it as-is for now, or pull multi-viewer support out of this PR and into a follow-up where it can then be fully developed? Screen.Recording.2024-04-24.at.7.38.02.AM.mov |
|
For the Science data dropdown, The SCI data has the associated DQ data. The ERR data does not. Should the ERR data populate in this dropdown if it doesn't have DQ data associated with it? Right now it does, and if I try to select it I get a TypeError. While it doesn't result in a traceback, with this DQ layer, Flags 0 and 9 are the two that are relevant. This is somewhat the same idea as above, but would you want to see all of the Flags even if they don't relate to the DQ that is loaded at the time, or would it be best to only display the flags that are relevant to the DQ? |
Screen.Recording.2024-04-25.at.10.16.35.AM.movIf I have SCI and ERR data loaded into one viewer, I deselect the DQ when SCI is selected, and then display ERR then back to SCI, the DQ comes back on automatically. Should this default to be turned off unless it is set to be on? |
My opinion isn't very strong, but I prefer that DQ visibility "follows" the science layer by default. I accept though if anyone feels that's inconsistent with the hidden-by-default approach in cubeviz.
This is a good point! I've updated the PR in 9e92b40 to limit the science layer data dropdown menu only to data layers that are "roots" (no parents) and that have children. That excludes ERR but includes SCI. In my view, the ERR array could (and one day should?) be considered a child of the science data, but we can save that for another day. |
If I am in the uncertainty-viewer, I am able to have the ERR data and overlay the DQ data. I cannot do this in the flux-viewer with the ERR/DQ combination. Let's say I do the same thing only with a Moment Map/DQ combo, in either viewer DQ won't display with it. I'm looking at the parent/child relationship now to see if I can pinpoint this. |
|
Good catch, @gibsongreen! The parent visibility changes were being propagated in some cases and not in the one you noticed. I've fixed the logic in the visibility in c5c0a28 to cover this case. |
There was a problem hiding this comment.
I really like the behavior now - thanks for iterating through all the feedback!
One thing I noticed (but isn't new in this PR), is that the tooltip to change the color says "Change the color of this list" - is "list" the right terminology here? Would "flag" be more intuitive?)
I also left a small number of small questions/suggestions for reusability - but many of them can probably be generalized later if needed.
| def is_image_viewer(viewer): | ||
| from jdaviz.configs.imviz.plugins.viewers import ImvizImageView | ||
| from jdaviz.configs.cubeviz.plugins.viewers import CubevizImageView | ||
|
|
||
| return isinstance(viewer, (ImvizImageView, CubevizImageView)) |
There was a problem hiding this comment.
might be nice to move this into the ViewerSelect directly. There already is an is_image_viewer defined, although it doesn't currently allow for cube viewers. So maybe we can check the current uses for that to see if that can be extended to include either, or if we want a separate is_image_or_cube_viewer?
| viewer_filter_names = [filt.__name__ for filt in self.viewer.filters] | ||
| if 'is_image_viewer' not in viewer_filter_names: | ||
| self.viewer.add_filter(is_image_viewer) | ||
| self.viewer._on_viewers_changed() |
There was a problem hiding this comment.
this is actually a bug in add_filters that I am also fixing in #2827.... if you want to do the same fix here (update add_filter to be self.filters = self.filters + [...] rather than +=) then we can remove this redundant line. We could also make add_filter smart enough to avoid duplication, and the you wouldn't need the if-statement of viewer_filter_names above at all 🤷♂️
There was a problem hiding this comment.
fix in the other PR is currently in this commit (although the link might break on a rebase):
- self.filters += [filter for filter in filters]
+ self.filters = self.filters + [filter for filter in filters]
There was a problem hiding this comment.
I've updated add_filter with your fix, and removed the _on_viewers_changed line above.
| selected_dq = self.dq_layer.selected_obj | ||
| selected_dq = self.get_dq_layers() |
There was a problem hiding this comment.
hmmm why doesn't selected_obj do what you want here?
There was a problem hiding this comment.
get_dq_layers did what I wanted and selected_obj didn't partly because I misunderstood the traceback that I was getting from selected_obj. It's clear to me now that this was because selected_obj can return types I wasn't expecting. In the next commit, I've added a property self.dq_layer_selected_flattened which just reorganizes the up the selected_obj output into the shape and types I expect.
| if self.filter_is_root: | ||
| # ignore layers that are children in associations: | ||
| def filter_is_root(data): | ||
| return self.app._get_assoc_data_parent(data.label) is None | ||
|
|
||
| # ignore layers that are children in associations: | ||
| self.add_filter(filter_is_root) |
There was a problem hiding this comment.
Just a note to self: I think this really needs to be refactored so that filters are passed directly to the init rather than all these boolean flags, but that can be follow-up since its not being introduced here (I also noticed this while working on #2827)
With this updated commit, I am still able to reproduce this screenshot |
|
After 95cf7f2, comments from @kecnry and @gibsongreen have been addressed. In particular, here's the updated behavior for the data menu, which prevents the user from selecting children of non-selected parent data, and explains itself: cubeviz-dq-data-menu.mov |
|
Thanks both for the helpful reviews! |
This PR follows #2767 to extend the DQ plugin for Cubeviz.
cubeviz-dq.mov
The plugin has only one difference from Imviz – it assumes that the
flux-vieweris the only viewer that will contain the DQ layer. Since the DQ layer will have the same dimensions as the cube, I wanted to make sure no one ever tries to use the cube DQ in the spectrum viewer, for example.The data dropdown menu offers the DQ layer as a child of the (single) cube loaded into
flux-viewer.Change log entry
CHANGES.rst? If you want to avoid merge conflicts,list the proposed change log here for review and add to
CHANGES.rstbefore merge. If no, maintainershould add a
no-changelog-entry-neededlabel.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.
triviallabel.