BUG: Fix get_subsets when XOR is involved
Description
This pull request is to fix bug found in https://github.com/spacetelescope/jdaviz/pull/2941#discussion_r1672866076
Change log entry
- [x] 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 toCHANGES.rstbefore merge. If no, maintainer should add ano-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.
- [x] Are two approvals required? Branch protection rule does not check for the second approval. If a second approval is not necessary, please apply the
triviallabel. - [x] Do the proposed changes actually accomplish desired goals? Also manually run the affected example notebooks, if necessary.
- [x] Do the proposed changes follow the STScI Style Guides?
- [x] Are tests added/updated as required? If so, do they follow the STScI Style Guides?
- [x] Are docs added/updated as required? If so, do they follow the STScI Style Guides?
- [x] Did the CI pass? If not, are the failures related?
- [x] 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. Bugfix milestone also needs an accompanying backport label.
- [ ] After merge, any internal documentations need updating (e.g., JIRA, Innerspace)? 🐱
Was hoping we could simply hook into glue-astronomy region translator but it didn't work. Maybe the translator is incomplete. I think we wrote a lot of the stuff focused on 2D image region, not spectral region.
from glue.config import subset_state_translator
dc = specviz_helper.app.data_collection
subsets = dc.subset_groups
subset = subsets[0]
handler = subset_state_translator.get_handler_for('astropy-regions')
handler.to_object(subset.subsets[0]) # traceback
I feel like the proper solution is to implement a new spectral region translator in glue-astronomy but that is way more "story points" than we have available.
If this is too controversial, other options:
- Drop XOR from possible options, as Jesse suggested.
- Implement proper translation over at
glue-astronomy.
Codecov Report
Attention: Patch coverage is 96.55172% with 2 lines in your changes missing coverage. Please review.
Project coverage is 88.78%. Comparing base (
31ac58f) to head (6cb9ef3). Report is 126 commits behind head on main.
| Files with missing lines | Patch % | Lines |
|---|---|---|
| jdaviz/app.py | 96.49% | 2 Missing :warning: |
Additional details and impacted files
@@ Coverage Diff @@
## main #3124 +/- ##
==========================================
- Coverage 88.81% 88.78% -0.04%
==========================================
Files 112 112
Lines 17427 17446 +19
==========================================
+ Hits 15478 15489 +11
- Misses 1949 1957 +8
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
I was still able to find a bug by doing the following order of spectral subsets (you can use PR #3082 to import the entire dictionary). I would expect a SpectralRegions object with 6 elements but app.get_subsets(simplify_spectral=True) only returns 2. The following is the return from app.get_subsets(simplify_spectral=False):
{'Subset 1': [{'name': 'RangeSubsetState',
'glue_state': 'AndState',
'region': Spectral Region, 1 sub-regions:
(5.667263706861359 um, 6.768955332154135 um) ,
'sky_region': None,
'subset_state': <glue.core.subset.RangeSubsetState at 0x245fb7719f0>},
{'name': 'RangeSubsetState',
'glue_state': 'AndNotState',
'region': Spectral Region, 1 sub-regions:
(6.01443541526278 um, 6.495846921545017 um) ,
'sky_region': None,
'subset_state': <glue.core.subset.RangeSubsetState at 0x245fb7737f0>},
{'name': 'RangeSubsetState',
'glue_state': 'XorState',
'region': Spectral Region, 1 sub-regions:
(5.491363409920771 um, 6.912452830362193 um) ,
'sky_region': None,
'subset_state': <glue.core.subset.RangeSubsetState at 0x245fb61d420>},
{'name': 'RangeSubsetState',
'glue_state': 'OrState',
'region': Spectral Region, 1 sub-regions:
(7.01428986482661 um, 7.24110871431554 um) ,
'sky_region': None,
'subset_state': <glue.core.subset.RangeSubsetState at 0x245fb61e680>},
{'name': 'RangeSubsetState',
'glue_state': 'OrState',
'region': Spectral Region, 1 sub-regions:
(5.051612596937036 um, 5.204368130975595 um) ,
'sky_region': None,
'subset_state': <glue.core.subset.RangeSubsetState at 0x245fb61d060>},
{'name': 'RangeSubsetState',
'glue_state': 'XorState',
'region': Spectral Region, 1 sub-regions:
(6.06535393249499 um, 6.968000303706421 um) ,
'sky_region': None,
'subset_state': <glue.core.subset.RangeSubsetState at 0x245fde88610>}]}
@javerbukh , good catch. I guess I misunderstood how SpectralRegion.invert() behaves in a more complicated situation. I added more tests, including one that is similar to what you pointed out above. I might still have missed something, so please re-review. Thanks!
"Understand" is a strong word...