-
Notifications
You must be signed in to change notification settings - Fork 73
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
Exclude subsets from WCS-only layer filter #3097
Exclude subsets from WCS-only layer filter #3097
Conversation
8511b66
to
8120ebc
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3097 +/- ##
==========================================
+ Coverage 88.73% 88.74% +0.01%
==========================================
Files 111 111
Lines 17262 17257 -5
==========================================
- Hits 15317 15315 -2
+ Misses 1945 1942 -3 ☔ View full report in Codecov by Sentry. |
jdaviz/utils.py
Outdated
return ( | ||
# WCS-only layers have a metadata label: | ||
getattr(state, 'meta', {}).get(_wcs_only_label, False) and | ||
|
||
# the above logic doesn't exclude 100% of subsets, so here we | ||
# ensure that no subsets make it through the filter: | ||
not is_subset | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wait, so subset.state.meta['_WCS_ONLY']
is sometimes set and True? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to simplify this whole function, but there were corner cases that got messy. The layers that get passed through LayerSelect
can have the following attributes: data
, layer
, or meta
:
Lines 244 to 252 in 23b3edd
# exclude WCS-only layers from the layer choices: | |
if hasattr(layer, 'layer'): | |
state = layer.layer | |
elif hasattr(layer, 'data'): | |
state = layer.data | |
elif hasattr(layer, 'meta'): | |
state = layer | |
else: | |
raise NotImplementedError |
In the case where the layer is a subset,
layer.data
corresponds to the subset's parent Data, which itself may have a meta['_WCS_ONLY']
. That's where the second check is necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm experimenting right now to see if this works without the second elif
. 🤞🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Phew, much simplified in b13c3bc.
8120ebc
to
b13c3bc
Compare
Sorry, I merged this before two approvals without first labeling as trivial. If anyone disagrees, I can revert or follow up with another PR. |
Backport PR #3097: Exclude subsets from WCS-only layer filter
Description
If you create a subset in Imviz while WCS-linked, the subset appears in the Orientation plugin's "orientation in viewer" dropdown. It does not appear in the data dropdown. This is an artifact of the way that WCS-only layers are filtered in
LayerSelect
. I've tweaked the filter to be sure to exclude subsets.Change log entry
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, maintainershould 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.
trivial
label.