jdaviz icon indicating copy to clipboard operation
jdaviz copied to clipboard

BUG: Fix get_subsets when XOR is involved

Open pllim opened this issue 1 year ago • 6 comments

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 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.

  • [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 trivial label.
  • [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)? 🐱

pllim avatar Jul 30 '24 21:07 pllim

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.

pllim avatar Jul 30 '24 22:07 pllim

If this is too controversial, other options:

  1. Drop XOR from possible options, as Jesse suggested.
  2. Implement proper translation over at glue-astronomy.

pllim avatar Jul 31 '24 19:07 pllim

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.

codecov[bot] avatar Jul 31 '24 19:07 codecov[bot]

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>}]}

image

javerbukh avatar Aug 02 '24 12:08 javerbukh

@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!

pllim avatar Aug 02 '24 21:08 pllim

"Understand" is a strong word...

pllim avatar Aug 05 '24 13:08 pllim