-
Notifications
You must be signed in to change notification settings - Fork 0
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 matched pan zoom #29
Changes from 2 commits
e2f36a5
f8ede98
89dfa63
d6f5525
fedbdf1
131a412
5c3d48b
ea13cea
b155027
3c5fb47
cd5aafc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1979,7 +1979,8 @@ def set_data_visibility(self, viewer_reference, data_label, visible=True, replac | |
layer_is_wcs_only = getattr(layer.layer, 'meta', {}).get(self._wcs_only_label, False) | ||
if layer.layer.data.label == data_label and layer_is_wcs_only: | ||
layer.visible = False | ||
viewer.state.wcs_only_layers.append(data_label) | ||
if data_label not in viewer.state.wcs_only_layers: | ||
viewer.state.wcs_only_layers.append(data_label) | ||
selected_items.pop(data_id) | ||
|
||
# Sets the plot axes labels to be the units of the most recently | ||
|
@@ -2254,7 +2255,7 @@ def _create_viewer_item(self, viewer, vid=None, name=None, reference=None): | |
'config': self.config, # give viewer access to app config/layout | ||
'data_open': False, | ||
'collapse': True, | ||
'reference': reference, | ||
'reference': reference or name or vid, | ||
'linked_by_wcs': linked_by_wcs, | ||
} | ||
|
||
|
@@ -2319,6 +2320,12 @@ def _on_new_viewer(self, msg, vid=None, name=None, add_layers_to_viewer=False): | |
ref_data = self._jdaviz_helper.default_viewer._obj.state.reference_data | ||
new_viewer_item['reference_data_label'] = getattr(ref_data, 'label', None) | ||
|
||
if ( | ||
getattr(ref_data, 'meta', {}).get('_WCS_ONLY', False) and | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it more consistent to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I had avoided doing that only because of circular imports, originally. But I've pushed this commit which imports the function within the method and dodges the circularity. |
||
hasattr(viewer, 'reference') and linked_by_wcs | ||
): | ||
viewer.state.reference_data = ref_data | ||
|
||
new_stack_item = self._create_stack_item( | ||
container='gl-stack', | ||
viewers=[new_viewer_item]) | ||
|
@@ -2333,14 +2340,13 @@ def _on_new_viewer(self, msg, vid=None, name=None, add_layers_to_viewer=False): | |
|
||
self.session.application.viewers.append(viewer) | ||
|
||
# Send out a toast message | ||
self.hub.broadcast(ViewerAddedMessage(vid, sender=self)) | ||
|
||
if add_layers_to_viewer: | ||
for layer_label in add_layers_to_viewer: | ||
self.add_data_to_viewer(viewer.reference, layer_label) | ||
if hasattr(viewer, 'reference'): | ||
self.add_data_to_viewer(viewer.reference, layer_label) | ||
|
||
viewer.state.reference_data = ref_data | ||
# Send out a toast message | ||
self.hub.broadcast(ViewerAddedMessage(vid, sender=self)) | ||
|
||
return viewer | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,7 +14,7 @@ | |
from jdaviz.core.events import ( | ||
LinkUpdatedMessage, ExitBatchLoadMessage, ChangeRefDataMessage, | ||
AstrowidgetMarkersChangedMessage, MarkersPluginUpdate, | ||
SnackbarMessage, AddDataToViewerMessage, ViewerAddedMessage | ||
SnackbarMessage, ViewerAddedMessage, AddDataMessage | ||
) | ||
from jdaviz.core.custom_traitlets import FloatHandleEmpty | ||
from jdaviz.core.registries import tray_registry | ||
|
@@ -130,7 +130,7 @@ def __init__(self, *args, **kwargs): | |
self.hub.subscribe(self, ViewerAddedMessage, | ||
handler=self._on_viewer_added) | ||
|
||
self.hub.subscribe(self, AddDataToViewerMessage, | ||
self.hub.subscribe(self, AddDataMessage, | ||
handler=self._on_data_add_to_viewer) | ||
|
||
self._update_layer_label_default() | ||
|
@@ -224,7 +224,7 @@ def _update_link(self, msg={}): | |
# load data into the viewer that are now compatible with the | ||
# new link type, remove data from the viewer that are now | ||
# incompatible: | ||
wcs_linked = self.link_type.selected.lower() == 'wcs' | ||
wcs_linked = self.link_type.selected == 'WCS' | ||
viewer_selected = self.app.get_viewer(self.viewer.selected) | ||
|
||
data_in_viewer = self.app.get_viewer(viewer_selected.reference).data() | ||
|
@@ -240,6 +240,9 @@ def _update_link(self, msg={}): | |
f"Data '{data.label}' does not have a valid WCS - removing from viewer.", | ||
sender=self, color="warning")) | ||
|
||
if wcs_linked: | ||
self._send_wcs_layers_to_all_viewers() | ||
|
||
self.linking_in_progress = False | ||
self._update_layer_label_default() | ||
|
||
|
@@ -351,36 +354,54 @@ def add_orientation(self, rotation_angle=None, east_left=None, label=None, | |
) | ||
|
||
# add orientation layer to all viewers: | ||
self._add_data_to_all_viewers(label) | ||
for viewer_ref in self.app._viewer_store: | ||
self._add_data_to_viewer(label, viewer_ref) | ||
|
||
if set_on_create: | ||
self.orientation._update_layer_items() | ||
self.orientation.selected = label | ||
|
||
def _add_data_to_all_viewers(self, data_label): | ||
for viewer_ref in self.app.get_viewer_reference_names(): | ||
layers = [ | ||
layer.label for layer in | ||
self.app.get_viewer(viewer_ref).layers | ||
] | ||
if data_label not in layers: | ||
self.app.add_data_to_viewer(viewer_ref, data_label) | ||
def _add_data_to_viewer(self, data_label, viewer_id): | ||
viewer = self.app.get_viewer_by_id(viewer_id) | ||
|
||
wcs_only_layers = viewer.state.wcs_only_layers | ||
if data_label not in wcs_only_layers: | ||
self.app.add_data_to_viewer(viewer_id, data_label) | ||
|
||
def _on_viewer_added(self, msg): | ||
for data_label in self.orientation.choices: | ||
self._add_data_to_all_viewers(data_label) | ||
self._send_wcs_layers_to_all_viewers(viewers_to_update=[msg._viewer_id]) | ||
|
||
@observe('viewer_items') | ||
def _send_wcs_layers_to_all_viewers(self, *args, **kwargs): | ||
if not hasattr(self, 'viewer'): | ||
return | ||
|
||
wcs_only_layers = self.app._jdaviz_helper.default_viewer.state.wcs_only_layers | ||
|
||
viewers_to_update = kwargs.get( | ||
'viewers_to_update', self.app.get_viewer_reference_names() | ||
) | ||
|
||
if hasattr(self, 'orientation') and len(self.orientation.choices): | ||
self.viewer.selected = msg._viewer_id | ||
self.orientation.selected = self.orientation.choices[0] | ||
for viewer_ref in viewers_to_update: | ||
for wcs_layer in wcs_only_layers: | ||
if wcs_layer not in self.app.get_viewer_by_id(viewer_ref).state.wcs_only_layers: | ||
self.app.add_data_to_viewer(viewer_ref, wcs_layer) | ||
|
||
self.viewer.selected = viewer_ref | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this will actually cycle the viewer select in the orientation plugin, right? Is that for the sake of having the choices update in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need that because we call There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. but There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The one time that this is necessary is when a new viewer is created and the WCS-only reference data have been added to the viewer but not yet selected as reference data. We first need to change the filter to "see" the orientation options, then we need to select the default orientation – I do this a few lines below (https://github.com/bmorris3/jdaviz/pull/29/files/e2f36a547e94cb0d38e45b807cb7dfba38c85122#diff-022bcba39f3fd68183229b5433146c2a21ac800db2778790152edc9a6b1b01caR398). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok, and in that case
in the for-loop and run for all (passed) viewers, we should instead have
Does that make sense? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I understand what you're going for, but this method is also used when new orientation layers are created. In that case, it adds the new orientation layer to each viewer, so that it can be selected as refdata in that viewer. So the function is written for the general case. |
||
|
||
self.orientation.set_wcs_only_filter(wcs_only=self.link_type_selected == 'WCS') | ||
# select the default orientation if no data have yet been | ||
# added to a newly created viewer: | ||
if ( | ||
base_wcs_layer_label in self.orientation.choices and | ||
not len(self.viewer.selected_obj.layers) | ||
): | ||
self.orientation.selected = base_wcs_layer_label | ||
|
||
def _on_data_add_to_viewer(self, msg): | ||
if ( | ||
msg._viewer_reference != 'imviz-0' and | ||
self.app.get_viewer_by_id(msg._viewer_reference).state.reference_data is None | ||
): | ||
self.viewer.selected = msg._viewer_reference | ||
self.orientation.selected = base_wcs_layer_label | ||
if msg._viewer_id != 'imviz-0': | ||
bmorris3 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if len(self.viewer.selected_obj.layers) == 1: | ||
# on adding first data layer, reset the limits: | ||
self.viewer.selected_obj.state.reset_limits() | ||
|
||
def vue_add_orientation(self, *args, **kwargs): | ||
self.add_orientation(set_on_create=True) | ||
|
@@ -392,8 +413,8 @@ def _change_reference_data(self, *args, **kwargs): | |
self.orientation.selected, viewer_id=self.viewer.selected | ||
) | ||
|
||
def _on_refdata_change(self, msg={}): | ||
self.orientation.only_wcs_layers = msg.data.meta.get('_WCS_ONLY', False) | ||
def _on_refdata_change(self, msg): | ||
self.orientation.set_wcs_only_filter(wcs_only=msg.data.meta.get('_WCS_ONLY', False)) | ||
if hasattr(self, 'viewer'): | ||
ref_data = self.ref_data | ||
viewer = self.app.get_viewer(self.viewer.selected) | ||
bmorris3 marked this conversation as resolved.
Show resolved
Hide resolved
bmorris3 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
@@ -406,10 +427,15 @@ def _on_refdata_change(self, msg={}): | |
elif not len(viewer.data()): | ||
self.link_type_selected = link_type_msg_to_trait['pixels'] | ||
|
||
if msg._data.label not in self.orientation.choices: | ||
return | ||
|
||
self.orientation.selected = msg._data.label | ||
bmorris3 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
# we never want to highlight subsets of pixels within WCS-only layers, | ||
# so if this layer is an ImageSubsetLayerState on a WCS-only layer, | ||
# ensure that it is never visible: | ||
for layer in viewer.state.layers: | ||
for layer in self.viewer.selected_obj.state.layers: | ||
bmorris3 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if ( | ||
hasattr(layer.layer, 'label') and | ||
layer.layer.label.startswith("Subset") and | ||
|
@@ -426,16 +452,15 @@ def ref_data(self): | |
@property | ||
def _refdata_change_available(self): | ||
viewer = self.app.get_viewer(self.viewer.selected) | ||
ref_data = self.ref_data | ||
selected_layer = [lyr.layer for lyr in viewer.layers | ||
if lyr.layer.label == self.orientation.selected] | ||
if len(selected_layer): | ||
is_subset = isinstance(selected_layer[0], (Subset, GroupedSubset)) | ||
else: | ||
is_subset = False | ||
return ( | ||
ref_data is not None and | ||
len(self.orientation.selected) and len(self.viewer.selected) and | ||
len(self.orientation.selected) and | ||
len(self.viewer.selected) and | ||
not is_subset | ||
) | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -51,7 +51,10 @@ def on_click(self, data): | |
if x is None or y is None: # Out of bounds | ||
return | ||
x, y, _, _ = self.viewer._get_real_xy(image, x, y) | ||
self.viewer.center_on((x, y)) | ||
if image.coords is not None: | ||
self.viewer.center_on(image.coords.pixel_to_world(x, y)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why this logic? It kinda defeats the purpose of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The use of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you want to switch the call to using sky coordinates, you don't need There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So do you want to see line 53 moved inside the conditional? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If this line makes the final cut, I think you can change the call to something like But I am not sure what you need to do to fix #29 (comment) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This line is already out of the final cut 👍🏻 |
||
else: | ||
self.viewer.center_on((x, y)) | ||
|
||
|
||
@viewer_tool | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why did the results change? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These test results have changed because the matched pan/zoom approach for data with WCS is slightly different than before, producing limits different by one pixel. The main differences come from trying to match zoom approximately even when viewers have different reference data. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have to think about this: When you give But then on L63-L62, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I still don't get this change. This test case is linked by pixel. Even though both images have WCS, it should not have mattered. No orientation layer was created. With this PR, when I run this test case interactively, this centered the image at t_normpan = v.toolbar.tools['jdaviz:imagepanzoom']
t_normpan.activate()
t_normpan.on_click({'event': 'click', 'domain': {'x': 1, 'y': 1}}) And if I have the pan/zoom tool active, where a single click would center to the pixel under cursor, that pixel is not actually the center when "pan to click location" is done. I do not think this change in test behavior is correct. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -9,7 +9,7 @@ | |||||
from glue.config import colormaps | ||||||
from glue.core import Data | ||||||
|
||||||
from jdaviz.configs.imviz.helper import get_top_layer_index | ||||||
from jdaviz.configs.imviz.helper import get_top_layer_index, get_reference_image_data | ||||||
from jdaviz.core.events import SnackbarMessage, AstrowidgetMarkersChangedMessage | ||||||
from jdaviz.core.helpers import data_has_valid_wcs | ||||||
|
||||||
|
@@ -178,8 +178,11 @@ def zoom_level(self): | |||||
raise ValueError('Viewer is still loading, try again later') | ||||||
|
||||||
if hasattr(self, '_get_real_xy'): | ||||||
i_top = get_top_layer_index(self) | ||||||
image = self.layers[i_top].layer | ||||||
if self.state.reference_data is not None: | ||||||
image, i_ref = get_reference_image_data(self.jdaviz_app, self.reference) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand this change. Zoom should be based on the visible layer, not the hidden reference data. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Why? If they're linked, is this distinction meaningful? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because I want the zoom level of the image under view, not zoom level of the reference data.
This comment was marked as outdated.
Sorry, something went wrong. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, no, it is completely wrong. I forgot to link by WCS when I commented 8 mins ago. When I link by WCS, the zoom_level is wrong. This change needs to be reverted. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
else: | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this |
||||||
i_top = get_top_layer_index(self) | ||||||
image = self.layers[i_top].layer | ||||||
real_min = self._get_real_xy(image, self.state.x_min, self.state.y_min) | ||||||
real_max = self._get_real_xy(image, self.state.x_max, self.state.y_max) | ||||||
else: | ||||||
|
@@ -210,8 +213,11 @@ def zoom_level(self, val): | |||||
|
||||||
new_dx = self.shape[1] * 0.5 / val | ||||||
if hasattr(self, '_get_real_xy'): | ||||||
i_top = get_top_layer_index(self) | ||||||
image = self.layers[i_top].layer | ||||||
if self.state.reference_data is not None: | ||||||
image, i_ref = get_reference_image_data(self.jdaviz_app, self.reference) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
else: | ||||||
i_top = get_top_layer_index(self) | ||||||
image = self.layers[i_top].layer | ||||||
real_min = self._get_real_xy(image, self.state.x_min, self.state.y_min) | ||||||
real_max = self._get_real_xy(image, self.state.x_max, self.state.y_max) | ||||||
cur_xcen = (real_min[0] + real_max[0]) * 0.5 | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1226,7 +1226,7 @@ class LayerSelect(SelectPluginComponent): | |
* register with all the automatic logic in the plugin's init by passing the string names | ||
of the respective traitlets. | ||
* use component in plugin template (see below) | ||
* refer to properties above based on the interally stored reference to the | ||
* refer to properties above based on the internally stored reference to the | ||
instantiated object of this component | ||
* observe the traitlets created and defined in the plugin, as necessary | ||
|
||
|
@@ -1265,7 +1265,6 @@ def __init__(self, plugin, items, selected, viewer, | |
``default`` text is provided but not in ``manual_options`` it will still be included as | ||
the first item in the list. | ||
""" | ||
|
||
super().__init__(plugin, | ||
items=items, | ||
selected=selected, | ||
|
@@ -1275,8 +1274,6 @@ def __init__(self, plugin, items, selected, viewer, | |
manual_options=manual_options, | ||
default_mode=default_mode) | ||
|
||
self.only_wcs_layers = only_wcs_layers | ||
|
||
self.hub.subscribe(self, AddDataMessage, | ||
handler=self._on_data_added) | ||
self.hub.subscribe(self, RemoveDataMessage, | ||
|
@@ -1295,6 +1292,7 @@ def __init__(self, plugin, items, selected, viewer, | |
self.add_observe(viewer, self._on_viewer_selected_changed) | ||
self.add_observe(selected, self._update_layer_items) | ||
self._update_layer_items() | ||
self.set_wcs_only_filter(only_wcs_layers) | ||
|
||
def _get_viewer(self, viewer): | ||
# newer will likely be the viewer name in most cases, but viewer id in the case | ||
|
@@ -1424,6 +1422,7 @@ def _update_layer_items(self, msg={}): | |
manual_items = [{'label': label} for label in self.manual_options] | ||
# use getattr so the super() call above doesn't try to access the attr before | ||
# it is initialized: | ||
|
||
if not getattr(self, 'only_wcs_layers', False): | ||
all_layers = [ | ||
layer for viewer in self.viewer_objs | ||
|
@@ -1448,7 +1447,7 @@ def _update_layer_items(self, msg={}): | |
layer_labels = [ | ||
layer.layer.label for layer in all_layers | ||
if self.app.state.layer_icons.get(layer.layer.label) or | ||
getattr(self, 'only_wcs_layers', False) | ||
self.only_wcs_layers | ||
] | ||
unique_layer_labels = list(set(layer_labels)) | ||
layer_items = [self._layer_to_dict(layer_label) for layer_label in unique_layer_labels] | ||
|
@@ -1463,6 +1462,22 @@ def _sort_by_icon(items_dict): | |
|
||
self._apply_default_selection() | ||
|
||
def set_wcs_only_filter(self, wcs_only): | ||
bmorris3 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
def is_wcs_only(data): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I feel like this re-implemented There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This needs to be a separate function that you can turn on and off as a |
||
return data.meta.get(self.app._wcs_only_label, False) | ||
|
||
filter_names = [getattr(filt, '__name__', '') for filt in self.filters] | ||
|
||
if not wcs_only and 'is_wcs_only' in filter_names: | ||
self.filters.remove(*[filt for filt in self.filters | ||
if getattr(filt, '__name__', '') == 'is_wcs_only']) | ||
elif wcs_only and 'is_wcs_only' not in filter_names: | ||
self.add_filter(is_wcs_only) | ||
|
||
@property | ||
def only_wcs_layers(self): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How is this different from |
||
return 'is_wcs_only' in [getattr(filt, '__name__', '') for filt in self.filters] | ||
|
||
@cached_property | ||
def selected_obj(self): | ||
viewer_names = self.viewer | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why was this needed (specifically for this PR - I agree we should try to merge these eventually, I just worry this might add more confusion since now we'll have "reference" in the viewer item that isn't necessarily the same as "reference" on the viewer itself).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is needed because when a new image viewer is created, there are hooks in place to set
vid
andname
, but notreference
(https://github.com/spacetelescope/jdaviz/blob/6372879516cdb96081e90b4b5ced404dc66448fe/jdaviz/configs/imviz/helper.py#L54C28-L56), but thereference
is the version used in some places.