From 7826a998d8447d6835b6598aee4d08b0b0997f33 Mon Sep 17 00:00:00 2001 From: "P. L. Lim" <2090236+pllim@users.noreply.github.com> Date: Thu, 8 Aug 2024 17:29:05 -0400 Subject: [PATCH] BUG: Fix get_subsets when XOR is involved (#3124) * TST: Modify test to expose bug in main * Fix the bug and add more tests * Maybe should not backport * Fix XOR logic flaw and add more tests. Co-authored-by: Jesse Averbukh * Simplify lo/hi logic Co-authored-by: Brett M. Morris --------- Co-authored-by: Jesse Averbukh Co-authored-by: Brett M. Morris --- CHANGES.rst | 2 + jdaviz/app.py | 132 +++++++++++------- .../plugins/subset_plugin/subset_plugin.py | 8 +- jdaviz/tests/test_subsets.py | 86 ++++++++++-- 4 files changed, 158 insertions(+), 70 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index 713480767f..b2f5d5668e 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -45,6 +45,8 @@ Mosviz Specviz ^^^^^^^ +- Fixed ``viz.app.get_subsets()`` for XOR mode. [#3124] + Specviz2d ^^^^^^^^^ diff --git a/jdaviz/app.py b/jdaviz/app.py index 546a7d48ef..018807d00b 100644 --- a/jdaviz/app.py +++ b/jdaviz/app.py @@ -1102,6 +1102,11 @@ def get_sub_regions(self, subset_state, simplify_spectral=True, two = self.get_sub_regions(subset_state.state2, simplify_spectral, use_display_units, get_sky_regions=get_sky_regions) + if simplify_spectral and isinstance(two, SpectralRegion): + merge_func = self._merge_overlapping_spectral_regions_worker + else: + def merge_func(spectral_region): # noop + return spectral_region if isinstance(one, list) and "glue_state" in one[0]: one[0]["glue_state"] = subset_state.__class__.__name__ @@ -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 - # subset regions are applied. - return one + two + return merge_func(one + two) elif subset_state.op is operator.and_: # This covers the AND subset mode @@ -1163,18 +1166,18 @@ def get_sub_regions(self, subset_state, simplify_spectral=True, return oppo.invert(one.lower, one.upper) else: - return two + one + return merge_func(two + one) elif subset_state.op is operator.or_: # This covers the ADD subset mode # one + two works for both Range and ROI subsets if one and two: - return two + one + return merge_func(two + one) elif one: return one elif two: return two elif subset_state.op is operator.xor: - # This covers the ADD subset mode + # This covers the XOR subset mode # Example of how this works, with "one" acting # as the XOR region and "two" as two ranges joined @@ -1198,34 +1201,55 @@ def get_sub_regions(self, subset_state, simplify_spectral=True, # (4.0 um, 4.5 um) (5.0 um, 6.0 um) (9.0 um, 12.0 um) if isinstance(two, SpectralRegion): + # This is the main application of XOR to other regions + if one.lower > two.lower and one.upper < two.upper: + if len(two) < 2: + inverted_region = one.invert(two.lower, two.upper) + else: + two_2 = None + for subregion in two: + temp_region = None + # No overlap + if subregion.lower > one.upper or subregion.upper < one.lower: + continue + temp_lo = max(subregion.lower, one.lower) + temp_hi = min(subregion.upper, one.upper) + temp_region = SpectralRegion(temp_lo, temp_hi) + if two_2: + two_2 += temp_region + else: + two_2 = temp_region + inverted_region = two_2.invert(one.lower, one.upper) + else: + inverted_region = two.invert(one.lower, one.upper) + new_region = None - temp_region = None for subregion in two: + temp_region = None # Add all subregions that do not intersect with XOR region # to a new SpectralRegion object if subregion.lower > one.upper or subregion.upper < one.lower: - if not new_region: - new_region = subregion - else: - new_region += subregion - # All other subregions are added to temp_region + temp_region = subregion + # Partial overlap + elif subregion.lower < one.lower and subregion.upper < one.upper: + temp_region = SpectralRegion(subregion.lower, one.lower) + elif subregion.upper > one.upper and subregion.lower > one.lower: + temp_region = SpectralRegion(one.upper, subregion.upper) + + if not temp_region: + continue + + if new_region: + new_region += temp_region else: - if not temp_region: - temp_region = subregion - else: - temp_region += subregion - # This is the main application of XOR to other regions - if not new_region: - new_region = temp_region.invert(one.lower, one.upper) - else: - new_region = new_region + temp_region.invert(one.lower, one.upper) + new_region = temp_region + # This adds the edge regions that are otherwise not included - if not (one.lower == temp_region.lower and one.upper == temp_region.upper): - new_region = new_region + one.invert(temp_region.lower, - temp_region.upper) - return new_region + if new_region: + return merge_func(new_region + inverted_region) + return inverted_region else: - return two + one + return merge_func(two + one) else: # This gets triggered in the InvertState case where state1 # is an object and state2 is None @@ -1319,14 +1343,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) + n_reg = len(spectral_region) + if not spectral_region or n_reg < 2: return False - for index in range(0, len(spectral_region) - 1): - if spectral_region[index].upper.value >= spectral_region[index + 1].lower.value: + for index in range(n_reg - 1): + if spectral_region[index]['region'].upper.value >= spectral_region[index + 1]['region'].lower.value: # noqa: E501 return True return False + @staticmethod + def _merge_overlapping_spectral_regions_worker(spectral_region): + if len(spectral_region) < 2: # noop + return spectral_region + + merged_regions = spectral_region[0] # Instantiate merged regions + for cur_reg in spectral_region[1:]: + # If the lower value of the current subregion is less than or equal to the upper + # value of the last subregion added to merged_regions, update last region in + # merged_regions with the max upper value between the two regions. + if cur_reg.lower <= merged_regions.upper: + merged_regions._subregions[-1] = ( + merged_regions._subregions[-1][0], + max(cur_reg.upper, merged_regions._subregions[-1][1])) + else: + merged_regions += cur_reg + return merged_regions + def merge_overlapping_spectral_regions(self, subset_name, att): """ Takes a spectral subset with subset_name and returns an ``OrState`` object @@ -1339,34 +1382,15 @@ def merge_overlapping_spectral_regions(self, subset_name, att): att : str Attribute that the subset uses to apply to data. """ - spectral_region = self.get_subsets(subset_name, spectral_only=True) - merged_regions = None - # Convert SpectralRegion object into a list with tuples representing - # the lower and upper values of each region. - reg_as_tup = [(sr.lower.value, sr.upper.value) for sr in spectral_region] - for index in range(0, len(spectral_region)): - # Instantiate merged regions - if not merged_regions: - merged_regions = [reg_as_tup[index]] - else: - last_merged = merged_regions[-1] - # If the lower value of the current subregion is less than or equal to the upper - # value of the last subregion added to merged_regions, update last_merged - # with the max upper value between the two regions. - if reg_as_tup[index][0] <= last_merged[1]: - last_merged = (last_merged[0], max(last_merged[1], reg_as_tup[index][1])) - merged_regions = merged_regions[:-1] - merged_regions.append(last_merged) - else: - merged_regions.append(reg_as_tup[index]) + merged_regions = self.get_subsets(subset_name, spectral_only=True) new_state = None for region in reversed(merged_regions): - convert_to_range = RangeSubsetState(region[0], region[1], att) - if new_state is None: - new_state = convert_to_range - else: + convert_to_range = RangeSubsetState(region.lower.value, region.upper.value, att) + if new_state: new_state = new_state | convert_to_range + else: + new_state = convert_to_range return new_state diff --git a/jdaviz/configs/default/plugins/subset_plugin/subset_plugin.py b/jdaviz/configs/default/plugins/subset_plugin/subset_plugin.py index d963bdd12d..50e8e092bc 100644 --- a/jdaviz/configs/default/plugins/subset_plugin/subset_plugin.py +++ b/jdaviz/configs/default/plugins/subset_plugin/subset_plugin.py @@ -305,11 +305,9 @@ def _unpack_get_subsets_for_ui(self): # types cannot be simplified so subsets contained that are skipped. if 'Mask' in self.subset_types: self.can_simplify = False - elif (len(self.subset_states) > 1 and isinstance(self.subset_states[0], RangeSubsetState) - and len(simplifiable_states - set(self.glue_state_types)) < 3): - self.can_simplify = True - elif (len(self.subset_states) > 1 and isinstance(self.subset_states[0], RangeSubsetState) - and self.app.is_there_overlap_spectral_subset(self.subset_selected)): + elif ((len(self.subset_states) > 1) and isinstance(self.subset_states[0], RangeSubsetState) + and ((len(simplifiable_states - set(self.glue_state_types)) < 3) + or self.app.is_there_overlap_spectral_subset(self.subset_selected))): self.can_simplify = True else: self.can_simplify = False diff --git a/jdaviz/tests/test_subsets.py b/jdaviz/tests/test_subsets.py index f7bef099a5..6f0bec6ce9 100644 --- a/jdaviz/tests/test_subsets.py +++ b/jdaviz/tests/test_subsets.py @@ -5,7 +5,7 @@ from astropy.utils.data import get_pkg_data_filename from glue.core.roi import CircularROI, CircularAnnulusROI, EllipticalROI, RectangularROI, XRangeROI from glue.core.subset_group import GroupedSubset -from glue.core.edit_subset_mode import AndMode, AndNotMode, OrMode, XorMode +from glue.core.edit_subset_mode import AndMode, AndNotMode, NewMode, OrMode, XorMode from regions import (PixCoord, CirclePixelRegion, CircleSkyRegion, RectanglePixelRegion, EllipsePixelRegion, CircleAnnulusPixelRegion) from numpy.testing import assert_allclose @@ -562,24 +562,88 @@ def test_edit_composite_spectral_subset(specviz_helper, spectrum1d): specviz_helper.app.get_subsets("Subset 1") -def test_edit_composite_spectral_with_xor(specviz_helper, spectrum1d): +def test_composite_spectral_with_xor(specviz_helper, spectrum1d): specviz_helper.load_data(spectrum1d) viewer = specviz_helper.app.get_viewer(specviz_helper._default_spectrum_viewer_reference_name) - viewer.apply_roi(XRangeROI(6400, 6600)) + viewer.apply_roi(XRangeROI(6200, 6800)) specviz_helper.app.session.edit_subset_mode.mode = OrMode - viewer.apply_roi(XRangeROI(7200, 7400)) - - viewer.apply_roi(XRangeROI(7600, 7800)) + viewer.apply_roi(XRangeROI(7200, 7600)) specviz_helper.app.session.edit_subset_mode.mode = XorMode - viewer.apply_roi(XRangeROI(6700, 7700)) + viewer.apply_roi(XRangeROI(6100, 7600)) reg = specviz_helper.app.get_subsets("Subset 1") - assert reg[0].lower.value == 6400 and reg[0].upper.value == 6600 - assert reg[1].lower.value == 6700 and reg[1].upper.value == 7200 - assert reg[2].lower.value == 7400 and reg[2].upper.value == 7600 - assert reg[3].lower.value == 7700 and reg[3].upper.value == 7800 + assert reg[0].lower.value == 6100 and reg[0].upper.value == 6200 + assert reg[1].lower.value == 6800 and reg[1].upper.value == 7200 + + specviz_helper.app.session.edit_subset_mode.mode = NewMode + viewer.apply_roi(XRangeROI(7000, 7200)) + specviz_helper.app.session.edit_subset_mode.mode = XorMode + viewer.apply_roi(XRangeROI(7100, 7300)) + specviz_helper.app.session.edit_subset_mode.mode = OrMode + viewer.apply_roi(XRangeROI(6900, 7105)) + reg = specviz_helper.app.get_subsets("Subset 2") + assert reg[0].lower.value == 6900 and reg[0].upper.value == 7105 + assert reg[1].lower.value == 7200 and reg[1].upper.value == 7300 + + specviz_helper.app.session.edit_subset_mode.mode = NewMode + viewer.apply_roi(XRangeROI(6000, 6500)) + specviz_helper.app.session.edit_subset_mode.mode = XorMode + viewer.apply_roi(XRangeROI(6100, 6200)) + reg = specviz_helper.app.get_subsets("Subset 3") + assert reg[0].lower.value == 6000 and reg[0].upper.value == 6100 + assert reg[1].lower.value == 6200 and reg[1].upper.value == 6500 + + specviz_helper.app.session.edit_subset_mode.mode = NewMode + viewer.apply_roi(XRangeROI(6100, 6200)) + specviz_helper.app.session.edit_subset_mode.mode = XorMode + viewer.apply_roi(XRangeROI(6000, 6500)) + reg = specviz_helper.app.get_subsets("Subset 4") + assert reg[0].lower.value == 6000 and reg[0].upper.value == 6100 + assert reg[1].lower.value == 6200 and reg[1].upper.value == 6500 + + specviz_helper.app.session.edit_subset_mode.mode = NewMode + viewer.apply_roi(XRangeROI(7500, 7600)) + specviz_helper.app.session.edit_subset_mode.mode = XorMode + viewer.apply_roi(XRangeROI(6000, 6010)) + reg = specviz_helper.app.get_subsets("Subset 5") + assert reg[0].lower.value == 6000 and reg[0].upper.value == 6010 + assert reg[1].lower.value == 7500 and reg[1].upper.value == 7600 + + +def test_composite_spectral_with_xor_complicated(specviz_helper, spectrum1d): + specviz_helper.load_data(spectrum1d) + viewer = specviz_helper.app.get_viewer(specviz_helper._default_spectrum_viewer_reference_name) + + viewer.apply_roi(XRangeROI(6100, 6700)) + + # (6100, 6200), (6300, 6700) + specviz_helper.app.session.edit_subset_mode.mode = AndNotMode + viewer.apply_roi(XRangeROI(6200, 6300)) + + # (6050, 6100), (6200, 6300), (6700, 6800) + specviz_helper.app.session.edit_subset_mode.mode = XorMode + viewer.apply_roi(XRangeROI(6050, 6800)) + + # (6050, 6100), (6200, 6300), (6700, 6800), (7000, 7200) + specviz_helper.app.session.edit_subset_mode.mode = OrMode + viewer.apply_roi(XRangeROI(7000, 7200)) + + # (6010, 6020), (6050, 6100), (6200, 6300), (6700, 6800), (7000, 7200) + viewer.apply_roi(XRangeROI(6010, 6020)) + + # (6010, 6020), (6050, 6090), (6100, 6200), (6300, 6700), (6800, 6850), (7000, 7200) + specviz_helper.app.session.edit_subset_mode.mode = XorMode + viewer.apply_roi(XRangeROI(6090, 6850)) + + reg = specviz_helper.app.get_subsets("Subset 1") + assert reg[0].lower.value == 6010 and reg[0].upper.value == 6020 + assert reg[1].lower.value == 6050 and reg[1].upper.value == 6090 + assert reg[2].lower.value == 6100 and reg[2].upper.value == 6200 + assert reg[3].lower.value == 6300 and reg[3].upper.value == 6700 + assert reg[4].lower.value == 6800 and reg[4].upper.value == 6850 + assert reg[5].lower.value == 7000 and reg[5].upper.value == 7200 def test_overlapping_spectral_regions(specviz_helper, spectrum1d):