generalize handling of composite subsets when determining subset type #2207
Conversation
kecnry
force-pushed
the
fix-composite-highlighting
branch
from
37b28cd to
8ff2297
Compare
|
|
||
| def get_subset_type(subset): | ||
| """ | ||
| Determine the subset type of a subset or layer |
There was a problem hiding this comment.
| Determine the subset type of a subset or layer | |
| Determine the subset type of a subset or layer. |
|
|
||
| Parameters | ||
| ---------- | ||
| subset : should have ``subset_state`` as an attribute, otherwise will return ``None``. |
There was a problem hiding this comment.
| subset : should have ``subset_state`` as an attribute, otherwise will return ``None``. | |
| subset | |
| Should have ``subset_state`` as an attribute, otherwise will return `None`. |
| subset_type : str or None | ||
| 'spatial', 'spectral', or None |
There was a problem hiding this comment.
| subset_type : str or None | |
| 'spatial', 'spectral', or None | |
| subset_type : { 'spatial', 'spectral', `None`} | |
| Subset type. |
| if isinstance(subset.subset_state, RoiSubsetState): | ||
| return 'spatial' | ||
| elif isinstance(subset.subset_state, RangeSubsetState): | ||
| return 'spectral' | ||
| else: | ||
| return None |
There was a problem hiding this comment.
Maybe this isn't correct, but the point is you want to break out of the loop as soon as you find a match, right?
| if isinstance(subset.subset_state, RoiSubsetState): | |
| return 'spatial' | |
| elif isinstance(subset.subset_state, RangeSubsetState): | |
| return 'spectral' | |
| else: | |
| return None | |
| if isinstance(subset.subset_state, RoiSubsetState): | |
| return 'spatial' | |
| elif isinstance(subset.subset_state, RangeSubsetState): | |
| return 'spectral' | |
| else: | |
| return None |
Also, what if what you are looking for is in state2? Can't we use Jesse's tree traversal here?
|
|
||
| while hasattr(subset.subset_state, 'state1'): | ||
| # this assumes no mixing between spatial and spectral subsets and just | ||
| # taking the first component (down the hierarchical tree) to determine the type | ||
| subset = subset.subset_state.state1 | ||
|
|
There was a problem hiding this comment.
Leaving this as a note to the future, this might be solved with:
subset = subset.subset_state
while hasattr(subset, 'state1'):
subset = subset.state1
|
From my testing this seems to be working as expected: composite spatial regions appear in the spectrum viewer and I am able to apply a composite spectral subset to that collapsed data. Was the work left on this to fix the test failure? |
|
Yes, this fixed the bug but resulted in other test failures that need to be resolved (or an alternate solution that does not cause test failures). |
|
superseded by #2266 |
Description
This pull request consolidates multiple different places in the code where subset type is determined and improves the logic to better handle composite subsets. This fixes the first bug found in #2182 (comment) .
Note that the layer-icon for a spatial subset in cubeviz can sometimes still show spectral until the next subset is created. That bug has a different cause (order of events firing) and is out of scope here.
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.