From 8e39c2e327b74f2c71df4e5cb53000c4e898a1f9 Mon Sep 17 00:00:00 2001 From: Thomas Robitaille Date: Fri, 10 May 2024 12:33:14 +0100 Subject: [PATCH 1/3] Make WCSLink.as_affine_link more robust to NaN values and raise an error if no overlap --- .../tests/test_wcs_autolinking.py | 26 +++++++++++++++++++ .../wcs_autolinking/wcs_autolinking.py | 17 +++++++++++- 2 files changed, 42 insertions(+), 1 deletion(-) diff --git a/glue/plugins/wcs_autolinking/tests/test_wcs_autolinking.py b/glue/plugins/wcs_autolinking/tests/test_wcs_autolinking.py index c3f02ba25..37f2445ee 100644 --- a/glue/plugins/wcs_autolinking/tests/test_wcs_autolinking.py +++ b/glue/plugins/wcs_autolinking/tests/test_wcs_autolinking.py @@ -591,3 +591,29 @@ def test_wcs_no_approximation(): with pytest.raises(NoAffineApproximation): link.as_affine_link(tolerance=0.1) + + +def test_no_wcs_overlap(): + + wcs1 = WCS(naxis=2) + wcs1.wcs.ctype = 'RA---TAN', 'DEC--TAN' + wcs1.wcs.crval = 10, 20 + wcs1.wcs.set() + + data1 = Data(label='Data 1') + data1.coords = wcs1 + data1['x'] = np.ones((2, 3)) + + wcs2 = WCS(naxis=2) + wcs2.wcs.ctype = 'RA---TAN', 'DEC--TAN' + wcs2.wcs.crval = 190, -20 + wcs2.wcs.set() + + data2 = Data(label='Data 2') + data2.coords = wcs2 + data2['x'] = np.ones((2, 3)) + + link = WCSLink(data1, data2) + + with pytest.raises(NoAffineApproximation, match='no overlap'): + link.as_affine_link() diff --git a/glue/plugins/wcs_autolinking/wcs_autolinking.py b/glue/plugins/wcs_autolinking/wcs_autolinking.py index 854abf706..8e44d997b 100644 --- a/glue/plugins/wcs_autolinking/wcs_autolinking.py +++ b/glue/plugins/wcs_autolinking/wcs_autolinking.py @@ -260,13 +260,28 @@ def as_affine_link(self, n_samples=1000, tolerance=1): # Convert to pixel positions in data2 pixel2 = self.forwards(*pixel1) + keep = np.ones(n_samples, dtype=bool) + for p in pixel1 + pixel2: + keep[np.isnan(p)] = False + + if not np.any(keep): + raise NoAffineApproximation(f'Could not find a good affine approximation to ' + f'WCSLink with tolerance={tolerance}, as no overlap') + + pixel1 = [p[keep] for p in pixel1] + pixel2 = [p[keep] for p in pixel2] + # First try simple offset def transform_offset(offsets): pixel1_tr = pixel1[0] - offsets[0], pixel1[1] - offsets[1] return np.hypot(pixel2[0] - pixel1_tr[0], pixel2[1] - pixel1_tr[1]) - best, _ = leastsq(transform_offset, (0, 0)) + best, status = leastsq(transform_offset, (0, 0)) + + if status not in [1, 2, 3, 4]: + raise NoAffineApproximation(f'Could not find a good affine approximation to ' + f'WCSLink with tolerance={tolerance}, as fitting failed') max_deviation = np.max(transform_offset(best)) From b02de2e759f8ba655237feb39997d997903d1891 Mon Sep 17 00:00:00 2001 From: Thomas Robitaille Date: Fri, 10 May 2024 14:02:34 +0100 Subject: [PATCH 2/3] Remove unnecessary check of fitting status and clarify docstring --- glue/plugins/wcs_autolinking/wcs_autolinking.py | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/glue/plugins/wcs_autolinking/wcs_autolinking.py b/glue/plugins/wcs_autolinking/wcs_autolinking.py index 8e44d997b..44abe524d 100644 --- a/glue/plugins/wcs_autolinking/wcs_autolinking.py +++ b/glue/plugins/wcs_autolinking/wcs_autolinking.py @@ -246,6 +246,9 @@ def as_affine_link(self, n_samples=1000, tolerance=1): For now this will only work for datasets in which two pixel coordinates are linked. + + The deviation to be compared to the tolerance is measured in the frame + of reference of the second dataset. """ if len(self.cids1) != 2 or len(self.cids2) != 2: @@ -277,11 +280,7 @@ def transform_offset(offsets): pixel1_tr = pixel1[0] - offsets[0], pixel1[1] - offsets[1] return np.hypot(pixel2[0] - pixel1_tr[0], pixel2[1] - pixel1_tr[1]) - best, status = leastsq(transform_offset, (0, 0)) - - if status not in [1, 2, 3, 4]: - raise NoAffineApproximation(f'Could not find a good affine approximation to ' - f'WCSLink with tolerance={tolerance}, as fitting failed') + best, _ = leastsq(transform_offset, (0, 0)) max_deviation = np.max(transform_offset(best)) From 0fcd1c453973ecd55ad54203a1046071dd1a811e Mon Sep 17 00:00:00 2001 From: Thomas Robitaille Date: Thu, 16 May 2024 14:25:22 +0100 Subject: [PATCH 3/3] Make sure that we don't do any unit parsing if the original and target unit are the same --- glue/core/units.py | 12 ++++++------ glue/viewers/image/tests/test_state.py | 13 +++++++++++++ 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/glue/core/units.py b/glue/core/units.py index 576772f6e..8090a4888 100644 --- a/glue/core/units.py +++ b/glue/core/units.py @@ -27,19 +27,19 @@ def to_unit(self, data, cid, values, target_units): if target_units is None: return values original_units = self._get_units(data, cid) - if original_units: - return self.converter_helper.to_unit(data, cid, values, original_units, target_units) - else: + if original_units == target_units or not original_units: return values + else: + return self.converter_helper.to_unit(data, cid, values, original_units, target_units) def to_native(self, data, cid, values, original_units): if original_units is None: return values target_units = self._get_units(data, cid) - if target_units: - return self.converter_helper.to_unit(data, cid, values, original_units, target_units) - else: + if original_units == target_units or not target_units: return values + else: + return self.converter_helper.to_unit(data, cid, values, original_units, target_units) def _get_units(self, data, cid): data = data.data if isinstance(data, Subset) else data diff --git a/glue/viewers/image/tests/test_state.py b/glue/viewers/image/tests/test_state.py index ce88efb0c..5ae4be992 100644 --- a/glue/viewers/image/tests/test_state.py +++ b/glue/viewers/image/tests/test_state.py @@ -458,3 +458,16 @@ def test_stretch_global(): assert layer_state.v_min == 49.95 assert layer_state.v_max == 949.05 + + +def test_attribute_units_invalid(): + + # Regression test for a bug that caused a crash if a dataset had an + # unrecognized unit + + viewer_state = ImageViewerState() + + data = Data(x=np.arange(100).reshape((10, 10))) + data.get_component('x').units = 'banana' + + ImageLayerState(layer=data, viewer_state=viewer_state)