From 8402f7a0d632c18fa2fe56f0ed0aba6b29ffefe1 Mon Sep 17 00:00:00 2001 From: gibsongreen Date: Thu, 11 Apr 2024 11:36:54 -0400 Subject: [PATCH 01/10] when footprint added before linking, make sure they display when link is updated --- jdaviz/configs/imviz/plugins/footprints/footprints.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/jdaviz/configs/imviz/plugins/footprints/footprints.py b/jdaviz/configs/imviz/plugins/footprints/footprints.py index cbc23995a9..1481f21be6 100644 --- a/jdaviz/configs/imviz/plugins/footprints/footprints.py +++ b/jdaviz/configs/imviz/plugins/footprints/footprints.py @@ -185,6 +185,12 @@ def _on_link_type_updated(self, msg=None): # toggle visibility as necessary self._on_is_active_changed() + # When fp(s) added via API before WCS link. Overlays visibility & is_active + # can be True, but only last fp will display. This ensures all fp(s) display + if not self.is_pixel_linked: + for choice in self.overlay.choices: + self.overlay.selected = choice + def vue_link_by_wcs(self, *args): # call other plugin so that other options (wcs_use_affine, wcs_use_fallback) # are retained. Remove this method if support for plotting footprints From 09baaf4d6cdf083e0cbf857cd363d1567dcf9917 Mon Sep 17 00:00:00 2001 From: gibsongreen Date: Thu, 11 Apr 2024 11:56:56 -0400 Subject: [PATCH 02/10] add change log --- CHANGES.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index 2e60768a29..722f4eed63 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -77,8 +77,8 @@ Cubeviz Imviz ^^^^^ - -- Fix a bug where footprints did not overlay when created via API. [#2790] +- Fix bugs where API created footprints did not overlay and only last + footprint displayed if added before linking. [#2790, #2797] - Improved behavior when orientations are created or selected without having data loaded in the viewer. [#2789] From 036bd0d42fda27ff5e3faad8c754c22ee5a83047 Mon Sep 17 00:00:00 2001 From: gibsongreen Date: Thu, 11 Apr 2024 15:36:03 -0400 Subject: [PATCH 03/10] add ability to restore overlay selection --- jdaviz/configs/imviz/plugins/footprints/footprints.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/jdaviz/configs/imviz/plugins/footprints/footprints.py b/jdaviz/configs/imviz/plugins/footprints/footprints.py index 1481f21be6..9764db0185 100644 --- a/jdaviz/configs/imviz/plugins/footprints/footprints.py +++ b/jdaviz/configs/imviz/plugins/footprints/footprints.py @@ -187,9 +187,11 @@ def _on_link_type_updated(self, msg=None): # When fp(s) added via API before WCS link. Overlays visibility & is_active # can be True, but only last fp will display. This ensures all fp(s) display + restore_choice = self.overlay.selected if not self.is_pixel_linked: for choice in self.overlay.choices: self.overlay.selected = choice + self.overlay.selected = restore_choice def vue_link_by_wcs(self, *args): # call other plugin so that other options (wcs_use_affine, wcs_use_fallback) From 3959b411f9624561fd2a85428fc0542b2efac74e Mon Sep 17 00:00:00 2001 From: Kyle Conroy Date: Fri, 12 Apr 2024 12:05:09 -0400 Subject: [PATCH 04/10] call _preset_args_changed instead of changing selection --- .../imviz/plugins/footprints/footprints.py | 23 +++++++++---------- 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/jdaviz/configs/imviz/plugins/footprints/footprints.py b/jdaviz/configs/imviz/plugins/footprints/footprints.py index 9764db0185..25dc5524e4 100644 --- a/jdaviz/configs/imviz/plugins/footprints/footprints.py +++ b/jdaviz/configs/imviz/plugins/footprints/footprints.py @@ -187,11 +187,9 @@ def _on_link_type_updated(self, msg=None): # When fp(s) added via API before WCS link. Overlays visibility & is_active # can be True, but only last fp will display. This ensures all fp(s) display - restore_choice = self.overlay.selected if not self.is_pixel_linked: for choice in self.overlay.choices: - self.overlay.selected = choice - self.overlay.selected = restore_choice + self._preset_args_changed(overlay_selected=choice) def vue_link_by_wcs(self, *args): # call other plugin so that other options (wcs_use_affine, wcs_use_fallback) @@ -469,24 +467,25 @@ def overlay_regions(self): return regs @observe('preset_selected', 'from_file', 'ra', 'dec', 'pa', 'v2_offset', 'v3_offset') - def _preset_args_changed(self, msg={}): + def _preset_args_changed(self, msg={}, overlay_selected=None): if self._ignore_traitlet_change: return - if not self.overlay_selected: + overlay_selected = overlay_selected if overlay_selected is not None else self.overlay_selected # noqa + if not overlay_selected: return name = msg.get('name', '').split('_selected')[0] - if self.overlay_selected not in self._overlays: + if overlay_selected not in self._overlays: # default dictionary has not been created yet return if len(name): - self._overlays[self.overlay_selected][name] = msg.get('new') - if name == 'from_file' and 'regions' in self._overlays[self.overlay_selected]: + self._overlays[overlay_selected][name] = msg.get('new') + if name == 'from_file' and 'regions' in self._overlays[overlay_selected]: # then the file may have been changed from the API, so we need to clear the cache # the cache will then be re-populated on the call to self.overlay_regions below - del self._overlays[self.overlay_selected]['regions'] + del self._overlays[overlay_selected]['regions'] regs = self.overlay_regions @@ -498,12 +497,12 @@ def _preset_args_changed(self, msg={}): wcs = getattr(viewer.state.reference_data, 'coords', None) if wcs is None: continue - existing_overlays = self._get_marks(viewer, self.overlay_selected) + existing_overlays = self._get_marks(viewer, overlay_selected) update_existing = len(existing_overlays) == len(regs) if not update_existing and len(existing_overlays): # clear any existing marks (length has changed, perhaps a new preset) viewer.figure.marks = [m for m in viewer.figure.marks - if getattr(m, 'overlay', None) != self.overlay_selected] + if getattr(m, 'overlay', None) != overlay_selected] # the following logic is adapted from # https://github.com/spacetelescope/jwst_novt/blob/main/jwst_novt/interact/display.py @@ -544,7 +543,7 @@ def _preset_args_changed(self, msg={}): else: mark = FootprintOverlay( viewer, - self.overlay_selected, + overlay_selected, x=x_coords, y=y_coords, colors=[self.color], From dfa0f762c973feb933eb062a2e898969c580f410 Mon Sep 17 00:00:00 2001 From: gibsongreen Date: Tue, 16 Apr 2024 11:42:42 -0400 Subject: [PATCH 05/10] internalize bug fix and add test coverage --- .../imviz/plugins/footprints/footprints.py | 19 +++++---- jdaviz/configs/imviz/tests/test_footprints.py | 42 +++++++++++++++++++ 2 files changed, 53 insertions(+), 8 deletions(-) diff --git a/jdaviz/configs/imviz/plugins/footprints/footprints.py b/jdaviz/configs/imviz/plugins/footprints/footprints.py index 25dc5524e4..2025c72e02 100644 --- a/jdaviz/configs/imviz/plugins/footprints/footprints.py +++ b/jdaviz/configs/imviz/plugins/footprints/footprints.py @@ -189,7 +189,8 @@ def _on_link_type_updated(self, msg=None): # can be True, but only last fp will display. This ensures all fp(s) display if not self.is_pixel_linked: for choice in self.overlay.choices: - self._preset_args_changed(overlay_selected=choice) + if len(self.overlay.choices) > 1: + self._change_overlay(overlay_selected=choice) def vue_link_by_wcs(self, *args): # call other plugin so that other options (wcs_use_affine, wcs_use_fallback) @@ -320,7 +321,7 @@ def vue_center_on_viewer(self, viewer_ref): self.center_on_viewer(viewer_ref) @observe('overlay_selected') - def _change_overlay(self, *args): + def _change_overlay(self, *args, overlay_selected=None): if not hasattr(self, 'overlay'): # pragma: nocover # plugin/traitlet startup return @@ -328,7 +329,9 @@ def _change_overlay(self, *args): # no overlay selected (this can happen when removing all overlays) return - if self.overlay_selected not in self._overlays: + overlay_selected = overlay_selected if overlay_selected is not None else self.overlay_selected # noqa + + if overlay_selected not in self._overlays: # create new entry with defaults (any defaults not provided here will be carried over # from the previous selection based on current traitlet values) @@ -336,21 +339,21 @@ def _change_overlay(self, *args): # to something other than the default, so we want to use that, otherwise for successive # new overlays, we want to ignore the traitlet and default back to "active" orange. default_color = '#c75109' if len(self._overlays) else self.color - self._overlays[self.overlay_selected] = {'color': default_color} + self._overlays[overlay_selected] = {'color': default_color} # similarly, if the user called any import APIs before opening the plugin, we want to # respect that, but when creating successive overlays, any selection from file/region # should be cleared for the next selection if self.preset_selected == 'From File...' and len(self._overlays) > 1: - self._overlays[self.overlay_selected]['from_file'] = '' - self._overlays[self.overlay_selected]['preset'] = self.preset.choices[0] + self._overlays[overlay_selected]['from_file'] = '' + self._overlays[overlay_selected]['preset'] = self.preset.choices[0] # for the first overlay only, default the position to be centered on the current # zoom limits of the first selected viewer if len(self._overlays) == 1 and len(self.viewer.selected): self.center_on_viewer(self.viewer.selected[0]) - fp = self._overlays[self.overlay_selected] + fp = self._overlays[overlay_selected] # we'll temporarily disable updating the overlays so that we can set all # traitlets simultaneously (and since we're only updating traitlets to a previously-set @@ -371,7 +374,7 @@ def _change_overlay(self, *args): # dict above. fp[key] = getattr(self, attr) self._ignore_traitlet_change = False - self._preset_args_changed() + self._preset_args_changed(overlay_selected=overlay_selected) def _mark_visible(self, viewer_id, overlay=None): if not self.is_active: diff --git a/jdaviz/configs/imviz/tests/test_footprints.py b/jdaviz/configs/imviz/tests/test_footprints.py index c9ef88051d..349bef4bc7 100644 --- a/jdaviz/configs/imviz/tests/test_footprints.py +++ b/jdaviz/configs/imviz/tests/test_footprints.py @@ -7,6 +7,7 @@ from jdaviz.core.marks import FootprintOverlay from jdaviz.configs.imviz.plugins.footprints.preset_regions import _all_apertures +from astropy.wcs import WCS pytest.importorskip("pysiaf") @@ -142,3 +143,44 @@ def test_user_api(imviz_helper, image_2d_wcs, tmp_path): assert plugin._obj.is_active is False viewer_marks = _get_markers_from_viewer(imviz_helper.default_viewer) assert viewer_marks[0].visible is False + + +def test_api_after_linking(imviz_helper): + # custom image to enable visual test in notebook + arr = np.ones(300, 300) + image_2d_wcs = WCS({'CTYPE1': 'RA---TAN', 'CUNIT1': 'deg', 'CDELT1': -0.0002777777778, + 'CRPIX1': 1, 'CRVAL1': 337.5202808, + 'CTYPE2': 'DEC--TAN', 'CUNIT2': 'deg', 'CDELT2': 0.0002777777778, + 'CRPIX2': 1, 'CRVAL2': -20.833333059999998}) + + viewer = imviz_helper.app.get_viewer_by_id('imviz-0') + + ndd = NDData(arr, wcs=image_2d_wcs) + imviz_helper.load_data(ndd) + imviz_helper.load_data(ndd) + + plugin = imviz_helper.plugins['Footprints'] + with plugin.as_active(): + + pointing = {'name': ['pt'], 'ra': [337.51], 'dec': [-20.77], 'pa': [0]} + plugin.color = 'green' + plugin.add_overlay(pointing['name'][0]) + plugin.ra = pointing['ra'][0] + plugin.dec = pointing['dec'][0] + plugin.pa = pointing['pa'][0] + + # when pixel linked are any marks displayed + viewer_marks = _get_markers_from_viewer(viewer) + no_marks_displayed = all(not mark.visible for mark in viewer_marks) + assert no_marks_displayed is True + + # link by wcs and retest + imviz_helper.link_data(link_type='wcs') + + viewer_marks = _get_markers_from_viewer(viewer) + # distinguish default from custom overlay with color + marks_displayed = any(mark.visible and mark.colors[0] == 'green' + for mark in viewer_marks) + + # when wcs linked are any marks displayed + assert marks_displayed is True From 8893c45d0a27cb6a434f19b714e9ee428356f4ef Mon Sep 17 00:00:00 2001 From: gibsongreen Date: Tue, 16 Apr 2024 13:44:16 -0400 Subject: [PATCH 06/10] add comment explaining link change loop --- jdaviz/configs/imviz/plugins/footprints/footprints.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/jdaviz/configs/imviz/plugins/footprints/footprints.py b/jdaviz/configs/imviz/plugins/footprints/footprints.py index 2025c72e02..0ad8ee5fca 100644 --- a/jdaviz/configs/imviz/plugins/footprints/footprints.py +++ b/jdaviz/configs/imviz/plugins/footprints/footprints.py @@ -187,10 +187,10 @@ def _on_link_type_updated(self, msg=None): # When fp(s) added via API before WCS link. Overlays visibility & is_active # can be True, but only last fp will display. This ensures all fp(s) display - if not self.is_pixel_linked: + if not self.is_pixel_linked and len(self.overlay.choices) > 1: for choice in self.overlay.choices: - if len(self.overlay.choices) > 1: - self._change_overlay(overlay_selected=choice) + # trigger the update without actually changing the user-selection + self._change_overlay(overlay_selected=choice) def vue_link_by_wcs(self, *args): # call other plugin so that other options (wcs_use_affine, wcs_use_fallback) From 43a684e011731b1a1d228d899b927dd6f7ef5466 Mon Sep 17 00:00:00 2001 From: gibsongreen Date: Tue, 16 Apr 2024 15:13:32 -0400 Subject: [PATCH 07/10] change fp to footprint in comments --- jdaviz/configs/imviz/plugins/footprints/footprints.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/jdaviz/configs/imviz/plugins/footprints/footprints.py b/jdaviz/configs/imviz/plugins/footprints/footprints.py index 0ad8ee5fca..e355d63072 100644 --- a/jdaviz/configs/imviz/plugins/footprints/footprints.py +++ b/jdaviz/configs/imviz/plugins/footprints/footprints.py @@ -185,8 +185,8 @@ def _on_link_type_updated(self, msg=None): # toggle visibility as necessary self._on_is_active_changed() - # When fp(s) added via API before WCS link. Overlays visibility & is_active - # can be True, but only last fp will display. This ensures all fp(s) display + # When footprint(s) are added via API before WCS link. Overlays visibility & is_active + # can be True, but only last footprint will display. This ensures all footprints display if not self.is_pixel_linked and len(self.overlay.choices) > 1: for choice in self.overlay.choices: # trigger the update without actually changing the user-selection From 0bf2e2366cfe9bbb7206f2631d11fd2cbdb6b6d1 Mon Sep 17 00:00:00 2001 From: gibsongreen Date: Wed, 17 Apr 2024 10:26:02 -0400 Subject: [PATCH 08/10] address case where default overlay is created in init --- jdaviz/configs/imviz/plugins/footprints/footprints.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/jdaviz/configs/imviz/plugins/footprints/footprints.py b/jdaviz/configs/imviz/plugins/footprints/footprints.py index e355d63072..fe5a9cf35d 100644 --- a/jdaviz/configs/imviz/plugins/footprints/footprints.py +++ b/jdaviz/configs/imviz/plugins/footprints/footprints.py @@ -187,10 +187,10 @@ def _on_link_type_updated(self, msg=None): # When footprint(s) are added via API before WCS link. Overlays visibility & is_active # can be True, but only last footprint will display. This ensures all footprints display - if not self.is_pixel_linked and len(self.overlay.choices) > 1: + if not self.is_pixel_linked: for choice in self.overlay.choices: # trigger the update without actually changing the user-selection - self._change_overlay(overlay_selected=choice) + self._change_overlay(overlay_selected=choice, center_the_overlay=False) def vue_link_by_wcs(self, *args): # call other plugin so that other options (wcs_use_affine, wcs_use_fallback) @@ -321,7 +321,7 @@ def vue_center_on_viewer(self, viewer_ref): self.center_on_viewer(viewer_ref) @observe('overlay_selected') - def _change_overlay(self, *args, overlay_selected=None): + def _change_overlay(self, *args, overlay_selected=None, center_the_overlay=True): if not hasattr(self, 'overlay'): # pragma: nocover # plugin/traitlet startup return @@ -332,6 +332,10 @@ def _change_overlay(self, *args, overlay_selected=None): overlay_selected = overlay_selected if overlay_selected is not None else self.overlay_selected # noqa if overlay_selected not in self._overlays: + # _on_link_type_updated() called in init but don't want to create + # the default overlay yet, since center_on_viewer() will cause a traceback + if not center_the_overlay: + return # create new entry with defaults (any defaults not provided here will be carried over # from the previous selection based on current traitlet values) From 1f240db8f63f5b03038412339852cedc3ce4e9b7 Mon Sep 17 00:00:00 2001 From: gibsongreen Date: Wed, 17 Apr 2024 10:58:36 -0400 Subject: [PATCH 09/10] change np.ones() to np.ones(()) for testing --- jdaviz/configs/imviz/tests/test_footprints.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/jdaviz/configs/imviz/tests/test_footprints.py b/jdaviz/configs/imviz/tests/test_footprints.py index 349bef4bc7..a2d8422a5c 100644 --- a/jdaviz/configs/imviz/tests/test_footprints.py +++ b/jdaviz/configs/imviz/tests/test_footprints.py @@ -147,7 +147,7 @@ def test_user_api(imviz_helper, image_2d_wcs, tmp_path): def test_api_after_linking(imviz_helper): # custom image to enable visual test in notebook - arr = np.ones(300, 300) + arr = np.ones((300, 300)) image_2d_wcs = WCS({'CTYPE1': 'RA---TAN', 'CUNIT1': 'deg', 'CDELT1': -0.0002777777778, 'CRPIX1': 1, 'CRVAL1': 337.5202808, 'CTYPE2': 'DEC--TAN', 'CUNIT2': 'deg', 'CDELT2': 0.0002777777778, From 40ed3a4f89d842f5633052c6a8f1a9a7536626f2 Mon Sep 17 00:00:00 2001 From: gibsongreen Date: Wed, 17 Apr 2024 11:44:22 -0400 Subject: [PATCH 10/10] trigger CI --- jdaviz/configs/imviz/tests/test_footprints.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/jdaviz/configs/imviz/tests/test_footprints.py b/jdaviz/configs/imviz/tests/test_footprints.py index a2d8422a5c..c30094e08e 100644 --- a/jdaviz/configs/imviz/tests/test_footprints.py +++ b/jdaviz/configs/imviz/tests/test_footprints.py @@ -146,7 +146,7 @@ def test_user_api(imviz_helper, image_2d_wcs, tmp_path): def test_api_after_linking(imviz_helper): - # custom image to enable visual test in notebook + # custom image to enable visual test in a notebook arr = np.ones((300, 300)) image_2d_wcs = WCS({'CTYPE1': 'RA---TAN', 'CUNIT1': 'deg', 'CDELT1': -0.0002777777778, 'CRPIX1': 1, 'CRVAL1': 337.5202808,