BUG: Fix get_subsets when XOR is involved #3124

pllim · 2024-07-30T21:18:44Z

Description

This pull request is to fix bug found in #2941 (comment)

Change log entry

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.

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.
Do the proposed changes actually accomplish desired goals? Also manually run the affected example notebooks, if necessary.
Do the proposed changes follow the STScI Style Guides?
Are tests added/updated as required? If so, do they follow the STScI Style Guides?
Are docs added/updated as required? If so, do they follow the STScI Style Guides?
Did the CI pass? If not, are the failures related?
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 · 2024-07-30T22:29:02Z

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 · 2024-07-31T17:38:22Z

jdaviz/tests/test_subsets.py


    specviz_helper.app.session.edit_subset_mode.mode = XorMode
-    viewer.apply_roi(XRangeROI(6700, 7700))
+    viewer.apply_roi(XRangeROI(6100, 7600))


The bug gets triggered when XOR is not exactly the same limits as the other regions (and that is usually the case in practice).

pllim · 2024-07-31T17:41:06Z

jdaviz/app.py

-                    merged_regions.append(last_merged)
-                else:
-                    merged_regions.append(reg_as_tup[index])
+        merged_regions = self.get_subsets(subset_name, spectral_only=True)


Not sure how controversial this is, but in order to fix the XOR bug, I basically have to move the "merge overlapping regions" calls into get_subsets when simplify_spectral=True. Otherwise, if you apply XOR and then another operator like OR, you get overlaps even if you ask for simplify_spectral=True, because to fix the bug, I have to chop up regions in the XOR operations or it is going to throw out some regions that do not overlap with the XOR limits. Hope this is not too confusing.

pllim · 2024-07-31T17:42:14Z

jdaviz/app.py

                return True
        return False

+    @staticmethod
+    def _merge_overlapping_spectral_regions_worker(spectral_region):


Same logic as before but I find operating on SpectralRegion directly less complicated than forcing tuple of floats. Also I need SpectralRegion directly to reuse it somewhere else.

pllim · 2024-07-31T17:43:25Z

jdaviz/app.py

@@ -1304,14 +1309,33 @@ def is_there_overlap_spectral_subset(self, subset_name):
        Returns True if the spectral subset with subset_name has overlapping
        subregions.
        """
-        spectral_region = self.get_subsets(subset_name, spectral_only=True)
-        if not spectral_region or len(spectral_region) < 2:
+        spectral_region = self.get_subsets(subset_name, simplify_spectral=False, spectral_only=True)


Because I have to move the "merge overlapping regions" calls into get_subsets when simplify_spectral=True, I have to force simplify_spectral=False here so this check is still correct.

pllim · 2024-07-31T17:44:22Z

jdaviz/app.py

@@ -1135,9 +1140,7 @@ def get_sub_regions(self, subset_state, simplify_spectral=True,
                    else:
                        if isinstance(two, list):
                            two[0]['glue_state'] = "AndNotState"
-                        # Return two first so that we preserve the chronology of how


Not sure what this comment is about because when you add two SpectralRegion, it automatically reorders by ascending wavelength, so it is never chronological to begin with. That is why this comment is removed.

pllim · 2024-07-31T19:26:52Z

If this is too controversial, other options:

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

codecov · 2024-07-31T19:42:17Z

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 ⚠️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

javerbukh · 2024-08-02T12:53:48Z

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

pllim · 2024-08-02T21:33:00Z

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

javerbukh

I'm so glad there is now at least one other person that understands this code 😄 Great job figuring out this bug, I could not replicate it with very complex composite subsets. Hopefully the second reviewer tries the same with similar results.

pllim · 2024-08-05T13:27:03Z

"Understand" is a strong word...

bmorris3

This looks good to me.

jdaviz/app.py

and add more tests. Co-authored-by: Jesse Averbukh <[email protected]>

Co-authored-by: Brett M. Morris <[email protected]>

pllim added bug Something isn't working 💤backport-v3.10.x on-merge: backport to v3.10.x labels Jul 30, 2024

pllim added this to the 3.10.4 milestone Jul 30, 2024

github-actions bot added the testing label Jul 30, 2024

pllim force-pushed the xor-xor-binks branch from 800aced to 6b14f41 Compare July 30, 2024 22:05

pllim force-pushed the xor-xor-binks branch from 6b14f41 to 93bc61a Compare July 30, 2024 22:36

pllim mentioned this pull request Jul 30, 2024

Create private method for importing spectral region objects #2941

Merged

9 tasks

pllim force-pushed the xor-xor-binks branch from 93bc61a to 0dd1bfb Compare July 31, 2024 17:36

pllim commented Jul 31, 2024

View reviewed changes

pllim force-pushed the xor-xor-binks branch from 0dd1bfb to 73c7aee Compare July 31, 2024 19:12

pllim removed the 💤backport-v3.10.x on-merge: backport to v3.10.x label Jul 31, 2024

pllim modified the milestones: 3.10.4, 4.0 Jul 31, 2024

pllim added the specviz label Jul 31, 2024

pllim marked this pull request as ready for review July 31, 2024 19:40

pllim requested review from rosteen, javerbukh, kecnry, bmorris3, haticekaratay, cshanahan1 and gibsongreen as code owners July 31, 2024 19:40

pllim force-pushed the xor-xor-binks branch from 6597007 to e995075 Compare August 2, 2024 21:31

javerbukh approved these changes Aug 5, 2024

View reviewed changes

pllim added the Ready for final review label Aug 5, 2024

bmorris3 approved these changes Aug 8, 2024

View reviewed changes

jdaviz/app.py Outdated Show resolved Hide resolved

pllim and others added 5 commits August 8, 2024 16:51

TST: Modify test to expose bug in main

37cfd25

Fix the bug and add more tests

a152b1c

Maybe should not backport

282cffd

Fix XOR logic flaw

be50276

and add more tests. Co-authored-by: Jesse Averbukh <[email protected]>

Simplify lo/hi logic

6cb9ef3

Co-authored-by: Brett M. Morris <[email protected]>

pllim force-pushed the xor-xor-binks branch from e995075 to 6cb9ef3 Compare August 8, 2024 20:56

pllim merged commit 7826a99 into spacetelescope:main Aug 8, 2024
16 checks passed

pllim deleted the xor-xor-binks branch August 8, 2024 21:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: Fix get_subsets when XOR is involved #3124

BUG: Fix get_subsets when XOR is involved #3124

pllim commented Jul 30, 2024 •

edited

Loading

pllim commented Jul 30, 2024

pllim Jul 31, 2024

pllim Jul 31, 2024

pllim Jul 31, 2024

pllim Jul 31, 2024

pllim Jul 31, 2024

pllim commented Jul 31, 2024

codecov bot commented Jul 31, 2024 •

edited

Loading

javerbukh commented Aug 2, 2024

pllim commented Aug 2, 2024

javerbukh left a comment

pllim commented Aug 5, 2024

bmorris3 left a comment

BUG: Fix get_subsets when XOR is involved #3124

BUG: Fix get_subsets when XOR is involved #3124

Conversation

pllim commented Jul 30, 2024 • edited Loading

Description

Change log entry

Checklist for package maintainer(s)

pllim commented Jul 30, 2024

pllim Jul 31, 2024

Choose a reason for hiding this comment

pllim Jul 31, 2024

Choose a reason for hiding this comment

pllim Jul 31, 2024

Choose a reason for hiding this comment

pllim Jul 31, 2024

Choose a reason for hiding this comment

pllim Jul 31, 2024

Choose a reason for hiding this comment

pllim commented Jul 31, 2024

codecov bot commented Jul 31, 2024 • edited Loading

Codecov Report

javerbukh commented Aug 2, 2024

pllim commented Aug 2, 2024

javerbukh left a comment

Choose a reason for hiding this comment

pllim commented Aug 5, 2024

bmorris3 left a comment

Choose a reason for hiding this comment

pllim commented Jul 30, 2024 •

edited

Loading

codecov bot commented Jul 31, 2024 •

edited

Loading