Skip to content

Commit

Permalink
Fix reparenting subsets between rotation orientations (#2734)
Browse files Browse the repository at this point in the history 
* Experimenting...

* Radius calculation now correct under rotation, trying to handle angle properly

* Account for rotation to update theta attribute

* Updated labels since we reparent to Orientation now. Numbers probably wrong

* Added correct calculations for rotated rectangle subset

* Codestyle

* Add to changelog

* Update jdaviz/configs/imviz/tests/test_delete_data.py

Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com>

* Remove extra pixel_to_pixel call

* Fixed bug with deleting second non-default orientation, debugging new angle with flip

* Fix flip handling, add modulo for angles

* Address review comments

Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com>

* Modulo should be in radians

* Apply suggestions from code review

Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com>

* Add test

* pllim review as commit

* Update jdaviz/app.py

---------

Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com>
  • Loading branch information
@rosteen @pllim
rosteen and pllim authored Mar 6, 2024
1 parent 4b9a114 commit 4433d39
Show file tree
Hide file tree
Showing 4 changed files with 120 additions and 42 deletions.
2 changes: 1 addition & 1 deletion CHANGES.rst
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,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]

Expand Down
92 changes: 60 additions & 32 deletions jdaviz/app.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -1763,6 +1762,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]
Expand Down Expand Up @@ -1796,11 +1796,22 @@ 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"):

# 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:
# 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

# Get the correct link to use for translation
roi = subset_state.roi
if type(roi) in (CircularROI, CircularAnnulusROI,
EllipticalROI, TrueCircularROI):
old_xc, old_yc = subset_state.center()
old_xc, old_yc = subset_state.center()
if isinstance(roi, (CircularROI, CircularAnnulusROI, EllipticalROI)):
# Convert center
x, y = pixel_to_pixel(old_parent.coords, new_parent.coords,
roi.xc, roi.yc)
Expand All @@ -1816,20 +1827,39 @@ 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)

elif type(roi) is RectangularROI:
x_min, y_min = 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,
roi.xmax, roi.ymax)
roi.xmin = x_min
roi.xmax = x_max
roi.ymin = y_min
roi.ymax = y_max

elif type(subset_group.subset_state) is RangeSubsetState:
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,
roi.xmin, roi.ymax)
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_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_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"):
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 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
Expand Down Expand Up @@ -1995,33 +2025,31 @@ def vue_data_item_remove(self, event):

data_label = event['item_name']
data = self.data_collection[data_label]
self._reparent_subsets(data)
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)

# Make sure the data isn't loaded in any viewers
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])
Expand Down
19 changes: 10 additions & 9 deletions jdaviz/configs/imviz/tests/test_delete_data.py
View file
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -65,18 +64,20 @@ 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 == "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)
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):
Expand Down
49 changes: 49 additions & 0 deletions jdaviz/configs/imviz/tests/test_orientation.py
View file
Original file line number Diff line number Diff line change
@@ -1,6 +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 regions import EllipseSkyRegion, RectangleSkyRegion

from jdaviz.configs.imviz.tests.utils import BaseImviz_WCS_WCS

Expand Down Expand Up @@ -152,3 +156,48 @@ 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)"


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

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: 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 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, 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)

0 comments on commit 4433d39

Please sign in to comment.