Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Fix Orientation selection/creation with no loaded data #2789

Merged
merged 9 commits into from
Apr 11, 2024
Merged
Show file tree
Hide file tree
Changes from all 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: 2 additions & 0 deletions CHANGES.rst
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,8 @@ Cubeviz
Imviz
^^^^^

- Improved behavior when orientations are created or selected without having data loaded in the viewer. [#2789]

Mosviz
^^^^^^

Expand Down
37 changes: 28 additions & 9 deletions jdaviz/configs/imviz/plugins/orientation/orientation.py
Original file line number Diff line number Diff line change
Expand Up @@ -275,6 +275,9 @@
def _get_wcs_angles(self, first_loaded_image=None):
if first_loaded_image is None:
first_loaded_image = self.viewer.selected_obj.first_loaded_data
if first_loaded_image is None:
# These won't end up getting used in this case, but we need an actual number
return 0, 0, 0
degn, dege, flip = get_compass_info(
first_loaded_image.coords, first_loaded_image.shape
)[-3:]
Expand Down Expand Up @@ -324,6 +327,12 @@
nothing will be done.

"""
self._add_orientation(rotation_angle=rotation_angle, east_left=east_left, label=label,
set_on_create=set_on_create, wrt_data=wrt_data)

def _add_orientation(self, rotation_angle=None, east_left=None, label=None,
set_on_create=True, wrt_data=None, from_ui=False):

if self.link_type_selected != 'WCS':
raise ValueError("must be aligned by WCS to add orientation options")

Expand All @@ -332,6 +341,12 @@
# default rotation:
wrt_data = self.viewer.selected_obj.first_loaded_data
if wrt_data is None: # Nothing in viewer
msg = "Viewer must have data loaded to add an orientation."
if from_ui:
Copy link
Contributor

Choose a reason for hiding this comment

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

Personally, I prefer more explicit name like raise_exception=False. From UI is just one of many possible reason why someone might want or not want an exception raised. But I'll leave it up to you.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not too worried about the semantics since this isn't exposed in the public API, it's only in the private method that we use internally now. I think I'll leave it as-is until a more general reason to raise an error here comes up. Thanks for the thought.

self.hub.broadcast(SnackbarMessage(msg, color="error",

Check warning on line 346 in jdaviz/configs/imviz/plugins/orientation/orientation.py

View check run for this annotation

Codecov / codecov/patch

jdaviz/configs/imviz/plugins/orientation/orientation.py#L346

Added line #L346 was not covered by tests
timeout=6000, sender=self))
else:
raise ValueError(msg)
return

rotation_angle = self.rotation_angle_deg(rotation_angle)
Expand Down Expand Up @@ -414,7 +429,7 @@
self.viewer.selected_obj.state.reset_limits()

def vue_add_orientation(self, *args, **kwargs):
self.add_orientation(set_on_create=True)
self._add_orientation(set_on_create=True, from_ui=True)

Check warning on line 432 in jdaviz/configs/imviz/plugins/orientation/orientation.py

View check run for this annotation

Codecov / codecov/patch

jdaviz/configs/imviz/plugins/orientation/orientation.py#L432

Added line #L432 was not covered by tests

@observe('orientation_layer_selected')
def _change_reference_data(self, *args, **kwargs):
Expand Down Expand Up @@ -486,35 +501,39 @@
if ref_data.label in self.orientation.choices:
self.orientation.selected = ref_data.label

def create_north_up_east_left(self, label="North-up, East-left", set_on_create=False):
def create_north_up_east_left(self, label="North-up, East-left", set_on_create=False,
from_ui=False):
"""
Set the rotation angle and flip to achieve North up and East left
according to the reference image WCS.
"""
if label not in self.orientation.choices:
degn = self._get_wcs_angles()[-3]
self.add_orientation(rotation_angle=degn, east_left=True,
label=label, set_on_create=set_on_create)
self._add_orientation(rotation_angle=degn, east_left=True,
label=label, set_on_create=set_on_create,
from_ui=from_ui)
elif set_on_create:
self.orientation.selected = label

def create_north_up_east_right(self, label="North-up, East-right", set_on_create=False):
def create_north_up_east_right(self, label="North-up, East-right", set_on_create=False,
from_ui=False):
"""
Set the rotation angle and flip to achieve North up and East right
according to the reference image WCS.
"""
if label not in self.orientation.choices:
degn = self._get_wcs_angles()[-3]
self.add_orientation(rotation_angle=180 - degn, east_left=False,
label=label, set_on_create=set_on_create)
self._add_orientation(rotation_angle=180 - degn, east_left=False,
label=label, set_on_create=set_on_create,
from_ui=from_ui)
elif set_on_create:
self.orientation.selected = label

def vue_select_north_up_east_left(self, *args, **kwargs):
self.create_north_up_east_left(set_on_create=True)
self.create_north_up_east_left(set_on_create=True, from_ui=True)

Check warning on line 533 in jdaviz/configs/imviz/plugins/orientation/orientation.py

View check run for this annotation

Codecov / codecov/patch

jdaviz/configs/imviz/plugins/orientation/orientation.py#L533

Added line #L533 was not covered by tests

def vue_select_north_up_east_right(self, *args, **kwargs):
self.create_north_up_east_right(set_on_create=True)
self.create_north_up_east_right(set_on_create=True, from_ui=True)

Check warning on line 536 in jdaviz/configs/imviz/plugins/orientation/orientation.py

View check run for this annotation

Codecov / codecov/patch

jdaviz/configs/imviz/plugins/orientation/orientation.py#L536

Added line #L536 was not covered by tests

def vue_select_default_orientation(self, *args, **kwargs):
self.orientation.selected = base_wcs_layer_label
Expand Down
24 changes: 24 additions & 0 deletions jdaviz/configs/imviz/tests/test_orientation.py
Original file line number Diff line number Diff line change
Expand Up @@ -223,3 +223,27 @@ def test_delete_orientation_with_subset(self, klass, angle, sbst_theta):
# 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)


class TestOrientationNoData(BaseImviz_WCS_WCS):
def test_create_no_data(self):
lc_plugin = self.imviz.plugins['Orientation']
lc_plugin.link_type = 'WCS'

self.imviz.create_image_viewer()
lc_plugin.viewer = "imviz-1"

with pytest.raises(ValueError, match="Viewer must have data loaded"):
lc_plugin._obj.create_north_up_east_left(set_on_create=True)

def test_select_no_data(self):
lc_plugin = self.imviz.plugins['Orientation']
lc_plugin.link_type = 'WCS'

lc_plugin._obj.create_north_up_east_left(set_on_create=True)

self.imviz.create_image_viewer()
lc_plugin.viewer = "imviz-1"
# This would error prior to bugfix
lc_plugin.orientation = "North-up, East-left"
self.imviz.app.add_data_to_viewer("imviz-1", "has_wcs_1[SCI,1]")
4 changes: 4 additions & 0 deletions jdaviz/core/freezable_state.py
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,10 @@ def reset_limits(self, *event):

x_min, x_max, y_min, y_max = self._get_reset_limits()

# If any bound wasn't set to a real value, don't update
if np.any(~np.isfinite([x_min, x_max, y_min, y_max])):
return

with delay_callback(self, 'x_min', 'x_max', 'y_min', 'y_max'):
self.x_min, self.x_max, self.y_min, self.y_max = x_min, x_max, y_min, y_max
# We need to adjust the limits in here to avoid triggering all
Expand Down