Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix reparenting subsets between rotation orientations #2734

Merged
merged 17 commits into from
Mar 6, 2024
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CHANGES.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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]

Expand Down
53 changes: 42 additions & 11 deletions jdaviz/app.py
Original file line number Diff line number Diff line change
Expand Up @@ -1763,6 +1763,7 @@
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 +1797,18 @@
# 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:]
rosteen marked this conversation as resolved.
Show resolved Hide resolved
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

Check warning on line 1805 in jdaviz/app.py

View check run for this annotation

Codecov / codecov/patch

jdaviz/app.py#L1805

Added line #L1805 was not covered by tests
rosteen marked this conversation as resolved.
Show resolved Hide resolved

# 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)
Expand All @@ -1816,18 +1824,37 @@
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
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_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")
rosteen marked this conversation as resolved.
Show resolved Hide resolved
setattr(roi, "theta", angle - np.deg2rad(relative_angle))
rosteen marked this conversation as resolved.
Show resolved Hide resolved

elif type(subset_group.subset_state) is RangeSubsetState:
range_state = subset_group.subset_state
Expand Down Expand Up @@ -1995,7 +2022,11 @@

data_label = event['item_name']
data = self.data_collection[data_label]
self._reparent_subsets(data)
if self.config == "imviz":
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also have to check link type here? You don't want this happening when linked by pixel, right? Or is the link type being handled somewhere else?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The check that link type is WCS is in _reparent_subsets.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we check something else other than the config? Maybe check if the orientation plugin is loaded (which could probably be done in one line to avoid building helper.plugins multiple times, or by accessing the tray_items which is probably cheaper)?

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:
Expand Down
16 changes: 7 additions & 9 deletions jdaviz/configs/imviz/tests/test_delete_data.py
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 @@ -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)
rosteen marked this conversation as resolved.
Show resolved Hide resolved
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