From db3548d4e339cf25852237ceca1e560b7ed99323 Mon Sep 17 00:00:00 2001 From: Kyle Conroy Date: Mon, 26 Feb 2024 09:55:27 -0500 Subject: [PATCH] Fix cloned phase-viewers when deleting/renaming an ephemeris component (#91) * regression test * fix ephemeris component rename/delete with a cloned phase viewer * add to existing changelog entry --- CHANGES.rst | 2 +- lcviz/plugins/ephemeris/ephemeris.py | 34 ++++++++++++++++------------ lcviz/tests/test_plugin_ephemeris.py | 21 ++++++++++++++++- 3 files changed, 40 insertions(+), 17 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index 330d9cf6..9828ff67 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -1,7 +1,7 @@ 0.2.0 - unreleased ------------------ -* Clone viewer tool. [#74] +* Clone viewer tool. [#74, #91] * Flux column plugin to choose which column is treated as the flux column for each dataset. [#77] diff --git a/lcviz/plugins/ephemeris/ephemeris.py b/lcviz/plugins/ephemeris/ephemeris.py index 1f509168..b6b62f59 100644 --- a/lcviz/plugins/ephemeris/ephemeris.py +++ b/lcviz/plugins/ephemeris/ephemeris.py @@ -155,10 +155,19 @@ def phase_viewer_id(self): return self._phase_viewer_id(self.component_selected) @property - def phase_viewer(self): + def default_phase_viewer(self): if not self.phase_viewer_exists: return None - return self.app.get_viewer(self.phase_viewer_id) + # we'll just treat the "default" as the first viewer connected to this + # ephemeris component + return self._get_phase_viewers()[0] + + def _get_phase_viewers(self, lbl=None): + if lbl is None: + lbl = self.component_selected + return [viewer for vid, viewer in self.app._viewer_store.items() + if isinstance(viewer, PhaseScatterView) + and viewer.ephemeris_component == lbl] @property def ephemerides(self): @@ -312,8 +321,7 @@ def vue_period_double(self, *args): self.period *= 2 def _check_if_phase_viewer_exists(self, *args): - viewer_base_refs = [id.split('[')[0] for id in self.app.get_viewer_ids()] - self.phase_viewer_exists = self.phase_viewer_id in viewer_base_refs + self.phase_viewer_exists = len(self._get_phase_viewers()) > 0 def _validate_component(self, lbl): if '[' in lbl or ']' in lbl: @@ -329,10 +337,10 @@ def _on_component_add(self, lbl): def _on_component_rename(self, old_lbl, new_lbl): # this is triggered when the plugin component detects a change to the component name self._ephemerides[new_lbl] = self._ephemerides.pop(old_lbl, {}) - if self._phase_viewer_id(old_lbl) in self.app.get_viewer_ids(): + for viewer in self._get_phase_viewers(old_lbl): self.app._update_viewer_reference_name( - self._phase_viewer_id(old_lbl), - self._phase_viewer_id(new_lbl), + viewer._ref_or_id, + viewer._ref_or_id.replace(old_lbl, new_lbl), update_id=True ) @@ -351,13 +359,9 @@ def _on_component_rename(self, old_lbl, new_lbl): def _on_component_remove(self, lbl): _ = self._ephemerides.pop(lbl, {}) - # remove the corresponding viewer, if it exists - viewer_item = self.app._viewer_item_by_id(self._phase_viewer_id(lbl)) - if viewer_item is None: # pragma: no cover - return - cid = viewer_item.get('id', None) - if cid is not None: - self.app.vue_destroy_viewer_item(cid) + # remove the corresponding viewer(s), if any exist + for viewer in self._get_phase_viewers(lbl): + self.app.vue_destroy_viewer_item(viewer._ref_or_id) self.hub.broadcast(EphemerisComponentChangedMessage(old_lbl=lbl, new_lbl=None, sender=self)) @@ -467,7 +471,7 @@ def round_to_1(x): if event.get('name') == 'wrap_at': old = event.get('old') if event.get('old') != '' else self._prev_wrap_at if event.get('new') != '': - pvs = self.phase_viewer.state + pvs = self.default_phase_viewer.state delta_phase = event.get('new') - old pvs.x_min, pvs.x_max = pvs.x_min + delta_phase, pvs.x_max + delta_phase # we need to cache the old value since it could become a string diff --git a/lcviz/tests/test_plugin_ephemeris.py b/lcviz/tests/test_plugin_ephemeris.py index d5e6cac9..6fd7d69e 100644 --- a/lcviz/tests/test_plugin_ephemeris.py +++ b/lcviz/tests/test_plugin_ephemeris.py @@ -36,7 +36,7 @@ def test_plugin_ephemeris(helper, light_curve_like_kepler_quarter): ephem._obj.vue_period_halve() assert ephem.period == 3.14 - pv = ephem._obj.phase_viewer + pv = ephem._obj.default_phase_viewer # original limits are set to 0->1 (technically 1-phase_wrap -> phase_wrap) assert (pv.state.x_min, pv.state.x_max) == (0.0, 1.0) ephem.wrap_at = 0.5 @@ -84,3 +84,22 @@ def test_plugin_ephemeris(helper, light_curve_like_kepler_quarter): # test that non-zero dpdt does not crash ephem.dpdt = 0.005 + + +def test_cloned_phase_viewer(helper, light_curve_like_kepler_quarter): + helper.load_data(light_curve_like_kepler_quarter) + ephem = helper.plugins['Ephemeris'] + + pv1 = ephem.create_phase_viewer() + pv2 = pv1._obj.clone_viewer() + assert len(helper.viewers) == 3 + assert pv1._obj.reference_id == 'flux-vs-phase:default' + assert pv2._obj.reference_id == 'flux-vs-phase:default[1]' + + # renaming ephemeris should update both labels + ephem.rename_component('default', 'renamed') + assert pv1._obj.reference_id == 'flux-vs-phase:renamed' + assert pv2._obj.reference_id == 'flux-vs-phase:renamed[1]' + + ephem.remove_component('renamed') + assert len(helper.viewers) == 1 # just flux-vs-phase