From e575800e36257689eb753a99f7dc595ac8654f32 Mon Sep 17 00:00:00 2001 From: Ricky O'Steen Date: Tue, 27 Feb 2024 11:26:58 -0500 Subject: [PATCH 01/17] Experimenting... --- jdaviz/app.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/jdaviz/app.py b/jdaviz/app.py index 5e6e8aea0d..cc0ebdb0bf 100644 --- a/jdaviz/app.py +++ b/jdaviz/app.py @@ -1993,9 +1993,14 @@ def set_data_visibility(self, viewer_reference, data_label, visible=True, replac def vue_data_item_remove(self, event): + print(event) data_label = event['item_name'] data = self.data_collection[data_label] - self._reparent_subsets(data) + if self.config == "imviz": + orient = self._jdaviz_helper.plugins["Orientation"].orientation.selected + self._reparent_subsets(data, new_parent=orient) + else: + self._reparent_subsets(data) # Make sure the data isn't loaded in any viewers for viewer_id in self._viewer_store: From 27ee9506b28db3a0e1378e1bcd06395f3dab7f2a Mon Sep 17 00:00:00 2001 From: Ricky O'Steen Date: Tue, 27 Feb 2024 17:12:15 -0500 Subject: [PATCH 02/17] Radius calculation now correct under rotation, trying to handle angle properly --- jdaviz/app.py | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/jdaviz/app.py b/jdaviz/app.py index cc0ebdb0bf..ca6acce983 100644 --- a/jdaviz/app.py +++ b/jdaviz/app.py @@ -1,3 +1,4 @@ +import math import operator import os import pathlib @@ -1763,6 +1764,7 @@ def _reparent_subsets(self, old_parent, new_parent=None): item in the data collection that doesn't match ``old_parent`` will be chosen. ''' from astropy.wcs.utils import pixel_to_pixel + from jdaviz.configs.imviz.wcs_utils import get_compass_info if isinstance(old_parent, str): old_parent = self.data_collection[old_parent] @@ -1796,6 +1798,14 @@ def _reparent_subsets(self, old_parent, new_parent=None): # Translate bounds through WCS if needed if (self.config == "imviz" and self._jdaviz_helper.plugins["Orientation"].link_type == "WCS"): + + old_angle, _, old_flip = get_compass_info(old_parent.coords, (10, 10))[-3:] + new_angle, _, new_flip = get_compass_info(new_parent.coords, (10, 10))[-3:] + relative_angle = new_angle - old_angle + print(old_angle, new_angle, relative_angle) + if old_flip != new_flip: + relative_angle += 180 + # Get the correct link to use for translation roi = subset_state.roi if type(roi) in (CircularROI, CircularAnnulusROI, @@ -1816,9 +1826,15 @@ def _reparent_subsets(self, old_parent, new_parent=None): dummy_x = old_xc + r x2, y2 = pixel_to_pixel(old_parent.coords, new_parent.coords, dummy_x, old_yc) - new_radius = np.abs(x2 - x) + # Need to use x and y in this radius calculation because the + # new orientation is likely rotated compared to the original. + new_radius = np.sqrt((x2 - x)**2 + (y2 - y)**2) setattr(roi, att, new_radius) + if hasattr(roi, "angle"): + angle = getattr(roi, "angle") + setattr(roi, "angle", angle + relative_angle) + elif type(roi) is RectangularROI: x_min, y_min = pixel_to_pixel(old_parent.coords, new_parent.coords, roi.xmin, roi.ymin) From fda63f914b5091a4246f926b14773d8211de3d18 Mon Sep 17 00:00:00 2001 From: Ricky O'Steen Date: Tue, 27 Feb 2024 17:22:01 -0500 Subject: [PATCH 03/17] Account for rotation to update theta attribute --- jdaviz/app.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/jdaviz/app.py b/jdaviz/app.py index ca6acce983..9ea4040e18 100644 --- a/jdaviz/app.py +++ b/jdaviz/app.py @@ -1831,9 +1831,9 @@ def _reparent_subsets(self, old_parent, new_parent=None): new_radius = np.sqrt((x2 - x)**2 + (y2 - y)**2) setattr(roi, att, new_radius) - if hasattr(roi, "angle"): - angle = getattr(roi, "angle") - setattr(roi, "angle", angle + relative_angle) + if hasattr(roi, "theta"): + angle = getattr(roi, "theta") + setattr(roi, "theta", angle - np.deg2rad(relative_angle)) elif type(roi) is RectangularROI: x_min, y_min = pixel_to_pixel(old_parent.coords, new_parent.coords, From 0ad2da6381597262e7cf77087623629f7899b901 Mon Sep 17 00:00:00 2001 From: Ricky O'Steen Date: Wed, 28 Feb 2024 11:44:36 -0500 Subject: [PATCH 04/17] Updated labels since we reparent to Orientation now. Numbers probably wrong --- jdaviz/app.py | 3 +-- jdaviz/configs/imviz/tests/test_delete_data.py | 16 +++++++--------- 2 files changed, 8 insertions(+), 11 deletions(-) diff --git a/jdaviz/app.py b/jdaviz/app.py index 9ea4040e18..ad16b5a8c6 100644 --- a/jdaviz/app.py +++ b/jdaviz/app.py @@ -1,4 +1,3 @@ -import math import operator import os import pathlib @@ -1799,7 +1798,7 @@ def _reparent_subsets(self, old_parent, new_parent=None): if (self.config == "imviz" and self._jdaviz_helper.plugins["Orientation"].link_type == "WCS"): - old_angle, _, old_flip = get_compass_info(old_parent.coords, (10, 10))[-3:] + old_angle, _, old_flip = get_compass_info(old_parent.coords, (10, 10))[-3:] new_angle, _, new_flip = get_compass_info(new_parent.coords, (10, 10))[-3:] relative_angle = new_angle - old_angle print(old_angle, new_angle, relative_angle) diff --git a/jdaviz/configs/imviz/tests/test_delete_data.py b/jdaviz/configs/imviz/tests/test_delete_data.py index b4b6c99c64..06cfd8eb31 100644 --- a/jdaviz/configs/imviz/tests/test_delete_data.py +++ b/jdaviz/configs/imviz/tests/test_delete_data.py @@ -1,5 +1,4 @@ import numpy as np -import pytest from astropy.coordinates import Angle from astropy.nddata import NDData from astropy.tests.helper import assert_quantity_allclose @@ -66,17 +65,16 @@ def test_delete_with_subset_wcs(self): assert len(self.imviz.app.data_collection.external_links) == 2 # Check that the reparenting and coordinate recalculations happened - assert subset1.subset_state.xatt.parent.label == "has_wcs_2[SCI,1]" - assert_allclose(subset1.subset_state.center(), (3, 2)) + assert subset1.subset_state.xatt.parent.label == "Default orientation" + assert_allclose(subset1.subset_state.center(), (1.75, 1.75)) - assert subset2.subset_state.xatt.parent.label == "has_wcs_2[SCI,1]" - assert_allclose(subset2.subset_state.roi.xmin, 1) - assert_allclose(subset2.subset_state.roi.ymin, 0, atol=1e-6) - assert_allclose(subset2.subset_state.roi.xmax, 3) - assert_allclose(subset2.subset_state.roi.ymax, 2) + assert subset2.subset_state.xatt.parent.label == "Default orientation" + assert_allclose(subset2.subset_state.roi.xmin, -0.25) + assert_allclose(subset2.subset_state.roi.ymin, -0.25, atol=1e-6) + assert_allclose(subset2.subset_state.roi.xmax, 1.75) + assert_allclose(subset2.subset_state.roi.ymax, 1.75) -@pytest.mark.xfail(reason="FIXME: JDAT-3958") class TestDeleteWCSLayerWithSubset(BaseImviz_WCS_GWCS): """Regression test for https://jira.stsci.edu/browse/JDAT-3958""" def test_delete_wcs_layer_with_subset(self): From 6ddbb29f2740a4928efec339ff1badf5be632566 Mon Sep 17 00:00:00 2001 From: Ricky O'Steen Date: Wed, 28 Feb 2024 17:06:32 -0500 Subject: [PATCH 05/17] Added correct calculations for rotated rectangle subset --- jdaviz/app.py | 39 ++++++++++++++++++++++++++------------- 1 file changed, 26 insertions(+), 13 deletions(-) diff --git a/jdaviz/app.py b/jdaviz/app.py index ad16b5a8c6..fed0b0c3d6 100644 --- a/jdaviz/app.py +++ b/jdaviz/app.py @@ -1801,15 +1801,14 @@ def _reparent_subsets(self, old_parent, new_parent=None): old_angle, _, old_flip = get_compass_info(old_parent.coords, (10, 10))[-3:] new_angle, _, new_flip = get_compass_info(new_parent.coords, (10, 10))[-3:] relative_angle = new_angle - old_angle - print(old_angle, new_angle, relative_angle) if old_flip != new_flip: relative_angle += 180 # Get the correct link to use for translation roi = subset_state.roi + old_xc, old_yc = subset_state.center() if type(roi) in (CircularROI, CircularAnnulusROI, EllipticalROI, TrueCircularROI): - old_xc, old_yc = subset_state.center() # Convert center x, y = pixel_to_pixel(old_parent.coords, new_parent.coords, roi.xc, roi.yc) @@ -1830,19 +1829,34 @@ def _reparent_subsets(self, old_parent, new_parent=None): new_radius = np.sqrt((x2 - x)**2 + (y2 - y)**2) setattr(roi, att, new_radius) - if hasattr(roi, "theta"): - angle = getattr(roi, "theta") - setattr(roi, "theta", angle - np.deg2rad(relative_angle)) - elif type(roi) is RectangularROI: - x_min, y_min = pixel_to_pixel(old_parent.coords, new_parent.coords, + x1, y1 = pixel_to_pixel(old_parent.coords, new_parent.coords, roi.xmin, roi.ymin) - x_max, y_max = pixel_to_pixel(old_parent.coords, new_parent.coords, + x2, y2 = pixel_to_pixel(old_parent.coords, new_parent.coords, + roi.xmin, roi.ymax) + x3, y3 = pixel_to_pixel(old_parent.coords, new_parent.coords, roi.xmax, roi.ymax) - roi.xmin = x_min - roi.xmax = x_max - roi.ymin = y_min - roi.ymax = y_max + x3, y3 = pixel_to_pixel(old_parent.coords, new_parent.coords, + roi.xmax, roi.ymin) + + # Calculate new width and height from possibly rotated result + new_width = np.sqrt((x3-x1)**2 + (y3-y1)**2) + new_height = np.sqrt((x2-x1)**2 + (y2-y1)**2) + + # Convert center + new_center = pixel_to_pixel(old_parent.coords, new_parent.coords, + old_xc, old_yc) + + # New min/max before applying theta + roi.xmin = new_center[0] - new_width/2 + roi.xmax = new_center[0] + new_width/2 + roi.ymin = new_center[1] - new_height/2 + roi.ymax = new_center[1] + new_height/2 + + # Account for rotation between orientations + if hasattr(roi, "theta"): + angle = getattr(roi, "theta") + setattr(roi, "theta", angle - np.deg2rad(relative_angle)) elif type(subset_group.subset_state) is RangeSubsetState: range_state = subset_group.subset_state @@ -2008,7 +2022,6 @@ def set_data_visibility(self, viewer_reference, data_label, visible=True, replac def vue_data_item_remove(self, event): - print(event) data_label = event['item_name'] data = self.data_collection[data_label] if self.config == "imviz": From 66ecdc126f632e9c1cfbff2f9fce1c75f397d6e7 Mon Sep 17 00:00:00 2001 From: Ricky O'Steen Date: Wed, 28 Feb 2024 17:07:55 -0500 Subject: [PATCH 06/17] Codestyle --- jdaviz/app.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/jdaviz/app.py b/jdaviz/app.py index fed0b0c3d6..9456547ad6 100644 --- a/jdaviz/app.py +++ b/jdaviz/app.py @@ -1831,13 +1831,13 @@ def _reparent_subsets(self, old_parent, new_parent=None): elif type(roi) is RectangularROI: x1, y1 = pixel_to_pixel(old_parent.coords, new_parent.coords, - roi.xmin, roi.ymin) + roi.xmin, roi.ymin) x2, y2 = pixel_to_pixel(old_parent.coords, new_parent.coords, - roi.xmin, roi.ymax) + roi.xmin, roi.ymax) x3, y3 = pixel_to_pixel(old_parent.coords, new_parent.coords, - roi.xmax, roi.ymax) + roi.xmax, roi.ymax) x3, y3 = pixel_to_pixel(old_parent.coords, new_parent.coords, - roi.xmax, roi.ymin) + roi.xmax, roi.ymin) # Calculate new width and height from possibly rotated result new_width = np.sqrt((x3-x1)**2 + (y3-y1)**2) @@ -1855,8 +1855,8 @@ def _reparent_subsets(self, old_parent, new_parent=None): # Account for rotation between orientations if hasattr(roi, "theta"): - angle = getattr(roi, "theta") - setattr(roi, "theta", angle - np.deg2rad(relative_angle)) + angle = getattr(roi, "theta") + setattr(roi, "theta", angle - np.deg2rad(relative_angle)) elif type(subset_group.subset_state) is RangeSubsetState: range_state = subset_group.subset_state From a11afe5dea8b53be62407acc918687d323c2061a Mon Sep 17 00:00:00 2001 From: Ricky O'Steen Date: Thu, 29 Feb 2024 09:45:29 -0500 Subject: [PATCH 07/17] Add to changelog --- CHANGES.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGES.rst b/CHANGES.rst index cd89220748..c9faa640ec 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -32,7 +32,7 @@ Imviz ^^^^^ - There is now option for image rotation in Orientation (was Links Control) plugin. - This feature requires WCS linking. [#2179, #2673, #2699] + This feature requires WCS linking. [#2179, #2673, #2699, #2734] - Add "Random" colormap for visualizing image segmentation maps. [#2671] From 4f6ad799b3b9deb94783f6fdb6e9929105bac414 Mon Sep 17 00:00:00 2001 From: Ricky O'Steen <39831871+rosteen@users.noreply.github.com> Date: Thu, 29 Feb 2024 10:58:07 -0500 Subject: [PATCH 08/17] Update jdaviz/configs/imviz/tests/test_delete_data.py Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com> --- jdaviz/configs/imviz/tests/test_delete_data.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/jdaviz/configs/imviz/tests/test_delete_data.py b/jdaviz/configs/imviz/tests/test_delete_data.py index 06cfd8eb31..72eb621503 100644 --- a/jdaviz/configs/imviz/tests/test_delete_data.py +++ b/jdaviz/configs/imviz/tests/test_delete_data.py @@ -70,7 +70,7 @@ def test_delete_with_subset_wcs(self): assert subset2.subset_state.xatt.parent.label == "Default orientation" assert_allclose(subset2.subset_state.roi.xmin, -0.25) - assert_allclose(subset2.subset_state.roi.ymin, -0.25, atol=1e-6) + assert_allclose(subset2.subset_state.roi.ymin, -0.25) assert_allclose(subset2.subset_state.roi.xmax, 1.75) assert_allclose(subset2.subset_state.roi.ymax, 1.75) From 9b44036145db9588f7a71cad3a9abda1e1c895b6 Mon Sep 17 00:00:00 2001 From: Ricky O'Steen Date: Thu, 29 Feb 2024 13:08:45 -0500 Subject: [PATCH 09/17] Remove extra pixel_to_pixel call --- jdaviz/app.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/jdaviz/app.py b/jdaviz/app.py index 9456547ad6..834b7adf54 100644 --- a/jdaviz/app.py +++ b/jdaviz/app.py @@ -1834,8 +1834,6 @@ def _reparent_subsets(self, old_parent, new_parent=None): roi.xmin, roi.ymin) x2, y2 = pixel_to_pixel(old_parent.coords, new_parent.coords, roi.xmin, roi.ymax) - x3, y3 = pixel_to_pixel(old_parent.coords, new_parent.coords, - roi.xmax, roi.ymax) x3, y3 = pixel_to_pixel(old_parent.coords, new_parent.coords, roi.xmax, roi.ymin) From aed4d48fd09dae6e5a033d1ce356419706359290 Mon Sep 17 00:00:00 2001 From: Ricky O'Steen Date: Mon, 4 Mar 2024 17:35:23 -0500 Subject: [PATCH 10/17] Fixed bug with deleting second non-default orientation, debugging new angle with flip --- jdaviz/app.py | 39 +++++++++++++++++++-------------------- 1 file changed, 19 insertions(+), 20 deletions(-) diff --git a/jdaviz/app.py b/jdaviz/app.py index 834b7adf54..c601cef980 100644 --- a/jdaviz/app.py +++ b/jdaviz/app.py @@ -1800,9 +1800,10 @@ def _reparent_subsets(self, old_parent, new_parent=None): old_angle, _, old_flip = get_compass_info(old_parent.coords, (10, 10))[-3:] new_angle, _, new_flip = get_compass_info(new_parent.coords, (10, 10))[-3:] - relative_angle = new_angle - old_angle if old_flip != new_flip: - relative_angle += 180 + relative_angle = new_angle + old_angle - 180 + else: + relative_angle = new_angle - old_angle # Get the correct link to use for translation roi = subset_state.roi @@ -1854,7 +1855,11 @@ def _reparent_subsets(self, old_parent, new_parent=None): # Account for rotation between orientations if hasattr(roi, "theta"): angle = getattr(roi, "theta") - setattr(roi, "theta", angle - np.deg2rad(relative_angle)) + if old_flip != new_flip: + new_angle = np.deg2rad(relative_angle) - angle + else: + new_angle = angle - np.deg2rad(relative_angle) + setattr(roi, "theta", new_angle) elif type(subset_group.subset_state) is RangeSubsetState: range_state = subset_group.subset_state @@ -2022,8 +2027,9 @@ def vue_data_item_remove(self, event): data_label = event['item_name'] data = self.data_collection[data_label] - if self.config == "imviz": - orient = self._jdaviz_helper.plugins["Orientation"].orientation.selected + orientation_plugin = self._jdaviz_helper.plugins.get("Orientation") + if orientation_plugin is not None: + orient = orientation_plugin.orientation.selected self._reparent_subsets(data, new_parent=orient) else: self._reparent_subsets(data) @@ -2032,27 +2038,20 @@ def vue_data_item_remove(self, event): for viewer_id in self._viewer_store: self.remove_data_from_viewer(viewer_id, data_label) - # Imviz has some extra logic below that can be skipped after data removal if we're not - # removing the reference data, so we check that here. - if self.config == "imviz": - imviz_refdata = False - ref_data, iref = self._jdaviz_helper.get_ref_data() - if data is ref_data: - imviz_refdata = True - self.data_collection.remove(self.data_collection[data_label]) - # If there are two or more datasets left we need to link them back together after removing - # the reference data (which would leave 0 external_links). - if len(self.data_collection) > 1 and len(self.data_collection.external_links) == 0: - if self.config == "imviz" and imviz_refdata: - link_type = self._jdaviz_helper.plugins["Orientation"].link_type.selected.lower() - self._jdaviz_helper.link_data(link_type=link_type) + # If there are two or more datasets left we need to link them back together if anything + # was linked only through the removed data. + if (len(self.data_collection) > 1 and + len(self.data_collection.external_links) < len(self.data_collection) - 1): + if orientation_plugin is not None: + orientation_plugin._obj._link_image_data() # Hack to restore responsiveness to imviz layers for viewer_ref in self.get_viewer_reference_names(): viewer = self.get_viewer(viewer_ref) loaded_layers = [layer.layer.label for layer in viewer.layers if - "Subset" not in layer.layer.label] + "Subset" not in layer.layer.label and layer.layer.label + not in orientation_plugin.orientation.labels] if len(loaded_layers): self.remove_data_from_viewer(viewer_ref, loaded_layers[-1]) self.add_data_to_viewer(viewer_ref, loaded_layers[-1]) From 9ca4d7dbfd6906a19d1f504d91cd2be000fcc209 Mon Sep 17 00:00:00 2001 From: Ricky O'Steen Date: Tue, 5 Mar 2024 10:47:27 -0500 Subject: [PATCH 11/17] Fix flip handling, add modulo for angles --- jdaviz/app.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/jdaviz/app.py b/jdaviz/app.py index c601cef980..f8302c37cf 100644 --- a/jdaviz/app.py +++ b/jdaviz/app.py @@ -1801,7 +1801,7 @@ def _reparent_subsets(self, old_parent, new_parent=None): old_angle, _, old_flip = get_compass_info(old_parent.coords, (10, 10))[-3:] new_angle, _, new_flip = get_compass_info(new_parent.coords, (10, 10))[-3:] if old_flip != new_flip: - relative_angle = new_angle + old_angle - 180 + relative_angle = 180 - new_angle - old_angle else: relative_angle = new_angle - old_angle @@ -1856,9 +1856,9 @@ def _reparent_subsets(self, old_parent, new_parent=None): if hasattr(roi, "theta"): angle = getattr(roi, "theta") if old_flip != new_flip: - new_angle = np.deg2rad(relative_angle) - angle + new_angle = (np.deg2rad(relative_angle) - angle) % 360 else: - new_angle = angle - np.deg2rad(relative_angle) + new_angle = (angle - np.deg2rad(relative_angle)) % 360 setattr(roi, "theta", new_angle) elif type(subset_group.subset_state) is RangeSubsetState: From 138309c17408704f40048979c549061f2265ce24 Mon Sep 17 00:00:00 2001 From: Ricky O'Steen Date: Tue, 5 Mar 2024 10:50:26 -0500 Subject: [PATCH 12/17] Address review comments Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com> --- jdaviz/app.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/jdaviz/app.py b/jdaviz/app.py index f8302c37cf..2da7258f29 100644 --- a/jdaviz/app.py +++ b/jdaviz/app.py @@ -1798,6 +1798,8 @@ def _reparent_subsets(self, old_parent, new_parent=None): if (self.config == "imviz" and self._jdaviz_helper.plugins["Orientation"].link_type == "WCS"): + # Default shape for WCS-only layers is 10x10, but it doesn't really matter + # since we only need the angles. old_angle, _, old_flip = get_compass_info(old_parent.coords, (10, 10))[-3:] new_angle, _, new_flip = get_compass_info(new_parent.coords, (10, 10))[-3:] if old_flip != new_flip: @@ -1859,7 +1861,7 @@ def _reparent_subsets(self, old_parent, new_parent=None): new_angle = (np.deg2rad(relative_angle) - angle) % 360 else: new_angle = (angle - np.deg2rad(relative_angle)) % 360 - setattr(roi, "theta", new_angle) + roi.theta = new_angle elif type(subset_group.subset_state) is RangeSubsetState: range_state = subset_group.subset_state From bd47489c8c8f5545fa2b26875f40638958b6c80c Mon Sep 17 00:00:00 2001 From: Ricky O'Steen Date: Tue, 5 Mar 2024 11:02:00 -0500 Subject: [PATCH 13/17] Modulo should be in radians --- jdaviz/app.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/jdaviz/app.py b/jdaviz/app.py index 2da7258f29..2424a524c2 100644 --- a/jdaviz/app.py +++ b/jdaviz/app.py @@ -1,3 +1,4 @@ +import math import operator import os import pathlib @@ -1858,9 +1859,9 @@ def _reparent_subsets(self, old_parent, new_parent=None): if hasattr(roi, "theta"): angle = getattr(roi, "theta") if old_flip != new_flip: - new_angle = (np.deg2rad(relative_angle) - angle) % 360 + new_angle = (np.deg2rad(relative_angle) - angle) % (2 * math.pi) else: - new_angle = (angle - np.deg2rad(relative_angle)) % 360 + new_angle = (angle - np.deg2rad(relative_angle)) % (2 * math.pi) roi.theta = new_angle elif type(subset_group.subset_state) is RangeSubsetState: From 052fa794d6f4f596113c59f60559ecb7cff98d9e Mon Sep 17 00:00:00 2001 From: Ricky O'Steen <39831871+rosteen@users.noreply.github.com> Date: Tue, 5 Mar 2024 13:50:37 -0500 Subject: [PATCH 14/17] Apply suggestions from code review Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com> --- jdaviz/app.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/jdaviz/app.py b/jdaviz/app.py index 2424a524c2..18a4f5bafb 100644 --- a/jdaviz/app.py +++ b/jdaviz/app.py @@ -1,4 +1,3 @@ -import math import operator import os import pathlib @@ -1857,11 +1856,12 @@ def _reparent_subsets(self, old_parent, new_parent=None): # Account for rotation between orientations if hasattr(roi, "theta"): - angle = getattr(roi, "theta") + angle = roi.theta if old_flip != new_flip: - new_angle = (np.deg2rad(relative_angle) - angle) % (2 * math.pi) + fac = 1.0 else: - new_angle = (angle - np.deg2rad(relative_angle)) % (2 * math.pi) + fac = -1.0 + new_angle = (fac * (np.deg2rad(relative_angle) - angle)) % (2 * np.pi) roi.theta = new_angle elif type(subset_group.subset_state) is RangeSubsetState: From b5b6c60a1199cb5fa08184d340d411f1fe67af32 Mon Sep 17 00:00:00 2001 From: Ricky O'Steen Date: Tue, 5 Mar 2024 15:42:37 -0500 Subject: [PATCH 15/17] Add test --- .../configs/imviz/tests/test_orientation.py | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/jdaviz/configs/imviz/tests/test_orientation.py b/jdaviz/configs/imviz/tests/test_orientation.py index 2dbcd53928..553ee40294 100644 --- a/jdaviz/configs/imviz/tests/test_orientation.py +++ b/jdaviz/configs/imviz/tests/test_orientation.py @@ -2,6 +2,8 @@ from astropy.table import Table from numpy.testing import assert_allclose +from glue.core.roi import EllipticalROI + from jdaviz.configs.imviz.tests.utils import BaseImviz_WCS_WCS @@ -152,3 +154,29 @@ def test_custom_orientation(self): lc_plugin._obj.add_orientation(rotation_angle=None, east_left=True, label=None, set_on_create=True, wrt_data=None) assert self.viewer.state.reference_data.label == "CCW 42.00 deg (E-left)" + + def test_delete_orientation_with_subset(self): + lc_plugin = self.imviz.plugins['Orientation'] + lc_plugin.link_type = 'WCS' + + # Should automatically be applied as reference to first viewer. + lc_plugin._obj.create_north_up_east_left(set_on_create=True) + + # Create rotated ellipse + self.imviz.app.get_viewer("imviz-0").apply_roi(EllipticalROI(3, 5, 1.2, 0.6, 0.5)) + + # Switch to N-up E-right + lc_plugin._obj.create_north_up_east_right(set_on_create=True) + + self.imviz.app.vue_data_item_remove({"item_name": "North-up, East-left"}) + + # Check that E-right still linked to default + assert len(self.imviz.app.data_collection.external_links) == 3 + assert self.imviz.app.data_collection.external_links[2].data1.label == "North-up, East-right" # noqa + assert self.imviz.app.data_collection.external_links[2].data2.label == "Default orientation" + + # Check that the subset got reparented and the angle is correct + subset_group = self.imviz.app.data_collection.subset_groups[0] + nuer_data = self.imviz.app.data_collection['North-up, East-right'] + assert subset_group.subset_state.xatt in nuer_data.components + assert_allclose(subset_group.subset_state.roi.theta, 2.641593, rtol=1e-5) From 65f3e10f8f02c735b1c20767b5864005387fc054 Mon Sep 17 00:00:00 2001 From: "P. L. Lim" <2090236+pllim@users.noreply.github.com> Date: Tue, 5 Mar 2024 18:41:36 -0500 Subject: [PATCH 16/17] pllim review as commit --- jdaviz/app.py | 29 ++++++--------- .../configs/imviz/tests/test_delete_data.py | 3 ++ .../configs/imviz/tests/test_orientation.py | 37 +++++++++++++++---- 3 files changed, 43 insertions(+), 26 deletions(-) diff --git a/jdaviz/app.py b/jdaviz/app.py index 18a4f5bafb..2a0a21eb3a 100644 --- a/jdaviz/app.py +++ b/jdaviz/app.py @@ -30,7 +30,6 @@ from glue_astronomy.spectral_coordinates import SpectralCoordinates from glue_astronomy.translators.regions import roi_subset_state_to_region from glue_jupyter.app import JupyterApplication -from glue_jupyter.bqplot.common.tools import TrueCircularROI from glue_jupyter.common.toolbar_vuetify import read_icon from glue_jupyter.state_traitlets_helpers import GlueState from ipypopout import PopoutButton @@ -1810,8 +1809,7 @@ def _reparent_subsets(self, old_parent, new_parent=None): # Get the correct link to use for translation roi = subset_state.roi old_xc, old_yc = subset_state.center() - if type(roi) in (CircularROI, CircularAnnulusROI, - EllipticalROI, TrueCircularROI): + if isinstance(roi, (CircularROI, CircularAnnulusROI, EllipticalROI)): # Convert center x, y = pixel_to_pixel(old_parent.coords, new_parent.coords, roi.xc, roi.yc) @@ -1832,7 +1830,7 @@ def _reparent_subsets(self, old_parent, new_parent=None): new_radius = np.sqrt((x2 - x)**2 + (y2 - y)**2) setattr(roi, att, new_radius) - elif type(roi) is RectangularROI: + elif isinstance(roi, RectangularROI): x1, y1 = pixel_to_pixel(old_parent.coords, new_parent.coords, roi.xmin, roi.ymin) x2, y2 = pixel_to_pixel(old_parent.coords, new_parent.coords, @@ -1841,30 +1839,25 @@ def _reparent_subsets(self, old_parent, new_parent=None): roi.xmax, roi.ymin) # Calculate new width and height from possibly rotated result - new_width = np.sqrt((x3-x1)**2 + (y3-y1)**2) - new_height = np.sqrt((x2-x1)**2 + (y2-y1)**2) + new_half_width = np.sqrt((x3-x1)**2 + (y3-y1)**2) * 0.5 + new_half_height = np.sqrt((x2-x1)**2 + (y2-y1)**2) * 0.5 # Convert center new_center = pixel_to_pixel(old_parent.coords, new_parent.coords, old_xc, old_yc) # New min/max before applying theta - roi.xmin = new_center[0] - new_width/2 - roi.xmax = new_center[0] + new_width/2 - roi.ymin = new_center[1] - new_height/2 - roi.ymax = new_center[1] + new_height/2 + roi.xmin = new_center[0] - new_half_width + roi.xmax = new_center[0] + new_half_width + roi.ymin = new_center[1] - new_half_height + roi.ymax = new_center[1] + new_half_height # Account for rotation between orientations if hasattr(roi, "theta"): - angle = roi.theta - if old_flip != new_flip: - fac = 1.0 - else: - fac = -1.0 - new_angle = (fac * (np.deg2rad(relative_angle) - angle)) % (2 * np.pi) - roi.theta = new_angle + fac = 1.0 if (old_flip != new_flip) else -1.0 + roi.theta = (fac * (np.deg2rad(relative_angle) - roi.theta)) % (2 * np.pi) # noqa: E501 - elif type(subset_group.subset_state) is RangeSubsetState: + elif isinstance(subset_group.subset_state, RangeSubsetState): range_state = subset_group.subset_state cur_unit = old_parent.coords.spectral_axis.unit new_unit = new_parent.coords.spectral_axis.unit diff --git a/jdaviz/configs/imviz/tests/test_delete_data.py b/jdaviz/configs/imviz/tests/test_delete_data.py index 72eb621503..936bc734c4 100644 --- a/jdaviz/configs/imviz/tests/test_delete_data.py +++ b/jdaviz/configs/imviz/tests/test_delete_data.py @@ -64,6 +64,9 @@ def test_delete_with_subset_wcs(self): # Make sure we re-linked images 2 and 3 (plus WCS-only reference data layer) assert len(self.imviz.app.data_collection.external_links) == 2 + # FIXME: 0.25 offset introduced by fake WCS layer, see + # https://jira.stsci.edu/browse/JDAT-4256 + # Check that the reparenting and coordinate recalculations happened assert subset1.subset_state.xatt.parent.label == "Default orientation" assert_allclose(subset1.subset_state.center(), (1.75, 1.75)) diff --git a/jdaviz/configs/imviz/tests/test_orientation.py b/jdaviz/configs/imviz/tests/test_orientation.py index 553ee40294..ac05c32477 100644 --- a/jdaviz/configs/imviz/tests/test_orientation.py +++ b/jdaviz/configs/imviz/tests/test_orientation.py @@ -1,8 +1,10 @@ import pytest +from astropy import units as u +from astropy.coordinates import SkyCoord from astropy.table import Table +from astropy.tests.helper import assert_quantity_allclose from numpy.testing import assert_allclose - -from glue.core.roi import EllipticalROI +from regions import EllipseSkyRegion, RectangleSkyRegion from jdaviz.configs.imviz.tests.utils import BaseImviz_WCS_WCS @@ -155,15 +157,25 @@ def test_custom_orientation(self): set_on_create=True, wrt_data=None) assert self.viewer.state.reference_data.label == "CCW 42.00 deg (E-left)" - def test_delete_orientation_with_subset(self): + +class TestDeleteOrientation(BaseImviz_WCS_WCS): + + @pytest.mark.parametrize("klass", [EllipseSkyRegion, RectangleSkyRegion]) + @pytest.mark.parametrize( + ("angle", "sbst_theta"), + [(0.5 * u.rad, 2.641593), + (-0.5 * u.rad, 3.641589)]) + def test_delete_orientation_with_subset(self, klass, angle, sbst_theta): lc_plugin = self.imviz.plugins['Orientation'] lc_plugin.link_type = 'WCS' # Should automatically be applied as reference to first viewer. lc_plugin._obj.create_north_up_east_left(set_on_create=True) - # Create rotated ellipse - self.imviz.app.get_viewer("imviz-0").apply_roi(EllipticalROI(3, 5, 1.2, 0.6, 0.5)) + # Create rotated shape + reg = klass(center=SkyCoord(ra=337.51931488, dec=-20.83187472, unit="deg"), + width=2.4 * u.arcsec, height=1.2 * u.arcsec, angle=angle) + self.imviz.load_regions(reg) # Switch to N-up E-right lc_plugin._obj.create_north_up_east_right(set_on_create=True) @@ -172,11 +184,20 @@ def test_delete_orientation_with_subset(self): # Check that E-right still linked to default assert len(self.imviz.app.data_collection.external_links) == 3 - assert self.imviz.app.data_collection.external_links[2].data1.label == "North-up, East-right" # noqa + assert self.imviz.app.data_collection.external_links[2].data1.label == "North-up, East-right" # noqa: E501 assert self.imviz.app.data_collection.external_links[2].data2.label == "Default orientation" - # Check that the subset got reparented and the angle is correct + # Check that the subset got reparented and the angle is correct in the display subset_group = self.imviz.app.data_collection.subset_groups[0] nuer_data = self.imviz.app.data_collection['North-up, East-right'] assert subset_group.subset_state.xatt in nuer_data.components - assert_allclose(subset_group.subset_state.roi.theta, 2.641593, rtol=1e-5) + assert_allclose(subset_group.subset_state.roi.theta, sbst_theta, rtol=1e-5) + + out_reg = self.imviz.app.get_subsets(include_sky_region=True)["Subset 1"][0]["sky_region"] + assert_allclose(out_reg.center.ra.deg, reg.center.ra.deg) + assert_allclose(out_reg.center.dec.deg, reg.center.dec.deg) + assert_quantity_allclose(out_reg.width, reg.width) + assert_quantity_allclose(out_reg.height, reg.height) + # FIXME: However, sky angle has to stay the same as per regions convention. + with pytest.raises(AssertionError, match="Not equal to tolerance"): + assert_quantity_allclose(out_reg.angle, reg.angle) From b01b2a6e97525a3cf62dfc6753ec2e7fd2817ccf Mon Sep 17 00:00:00 2001 From: Ricky O'Steen <39831871+rosteen@users.noreply.github.com> Date: Wed, 6 Mar 2024 10:21:58 -0500 Subject: [PATCH 17/17] Update jdaviz/app.py --- jdaviz/app.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/jdaviz/app.py b/jdaviz/app.py index 2a0a21eb3a..cc0400d22a 100644 --- a/jdaviz/app.py +++ b/jdaviz/app.py @@ -1802,6 +1802,8 @@ def _reparent_subsets(self, old_parent, new_parent=None): old_angle, _, old_flip = get_compass_info(old_parent.coords, (10, 10))[-3:] new_angle, _, new_flip = get_compass_info(new_parent.coords, (10, 10))[-3:] if old_flip != new_flip: + # Note that this won't work for an irregular/assymetric region if we + # ever implement those. relative_angle = 180 - new_angle - old_angle else: relative_angle = new_angle - old_angle