From 146771658df46fa8fe51b49fb7123b01d10f645b Mon Sep 17 00:00:00 2001 From: Thomas Robitaille Date: Tue, 17 Sep 2024 10:54:07 +0100 Subject: [PATCH 1/7] Avoid broadcasting a create subset message with an empty subset state followed by an update message, instead just broadcast a create subset message with the correct subset state --- glue/core/edit_subset_mode.py | 6 +++- glue/core/tests/test_edit_subset_mode.py | 46 +++++++++++++++++++++++- 2 files changed, 50 insertions(+), 2 deletions(-) diff --git a/glue/core/edit_subset_mode.py b/glue/core/edit_subset_mode.py index 9eb83ff83..3784f8cf0 100644 --- a/glue/core/edit_subset_mode.py +++ b/glue/core/edit_subset_mode.py @@ -71,7 +71,11 @@ def _combine_data(self, new_state, override_mode=None): if self.data_collection is None: raise RuntimeError("Must set data_collection before " "calling update") - self.edit_subset = [self.data_collection.new_subset_group()] + # We set the subset state directly here to avoid double messages + # (create then update). As the subset is new at this point, + # it doesn't make sense to apply a mode in any case. + self.edit_subset = [self.data_collection.new_subset_group(subset_state=new_state.copy())] + return subs = self._edit_subset for s in as_list(subs): mode(s, new_state) diff --git a/glue/core/tests/test_edit_subset_mode.py b/glue/core/tests/test_edit_subset_mode.py index 487ae395e..09dd3ec56 100644 --- a/glue/core/tests/test_edit_subset_mode.py +++ b/glue/core/tests/test_edit_subset_mode.py @@ -1,12 +1,19 @@ # pylint: disable=I0011,W0613,W0201,W0212,E1101,E1103 +import operator +from unittest.mock import MagicMock + import numpy as np +from ..application_base import Application +from ..hub import HubListener +from ..message import SubsetCreateMessage, SubsetUpdateMessage +from ..command import ApplySubsetState from ..data import Component, Data from ..data_collection import DataCollection from ..edit_subset_mode import (EditSubsetMode, ReplaceMode, OrMode, AndMode, XorMode, AndNotMode) -from ..subset import ElementSubsetState +from ..subset import ElementSubsetState, InequalitySubsetState class TestEditSubsetMode(object): @@ -87,3 +94,40 @@ def test_combines_make_copy(self): self.edit_mode.edit_subset = self.data.new_subset() self.edit_mode.update(self.data, self.state2) assert self.edit_mode.edit_subset.subset_state is not self.state2 + + +def test_no_double_messaging(): + + # Make sure that when we create a new subset via EditSubsetMode, we don't + # get two separate messages for creation and updating, but instead just a + # single create message with the right subset state. + + handler = MagicMock() + + app = Application() + + class Listener(HubListener): + pass + + listener = Listener() + + app.session.hub.subscribe(listener, SubsetCreateMessage, handler=handler) + app.session.hub.subscribe(listener, SubsetUpdateMessage, handler=handler) + + data = Data(x=[1, 2, 3, 4, 5]) + + app.data_collection.append(data) + + cmd = ApplySubsetState(data_collection=app.data_collection, + subset_state=data.id['x'] >= 3, + override_mode=ReplaceMode) + + app.session.command_stack.do(cmd) + + assert handler.call_count == 1 + message = handler.call_args[0][0] + assert isinstance(message, SubsetCreateMessage) + assert isinstance(message.subset.subset_state, InequalitySubsetState) + assert message.subset.subset_state.left is data.id['x'] + assert message.subset.subset_state.operator is operator.ge + assert message.subset.subset_state.right == 3 From 36b74e2eab081b3b27bbca15fcea5d36eff80a61 Mon Sep 17 00:00:00 2001 From: Thomas Robitaille Date: Mon, 6 Jan 2025 14:00:55 +0000 Subject: [PATCH 2/7] Added test to make sure that all subsets in different datasets have been created before message is emitted --- glue/core/tests/test_edit_subset_mode.py | 31 ++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/glue/core/tests/test_edit_subset_mode.py b/glue/core/tests/test_edit_subset_mode.py index 09dd3ec56..db65bdc71 100644 --- a/glue/core/tests/test_edit_subset_mode.py +++ b/glue/core/tests/test_edit_subset_mode.py @@ -131,3 +131,34 @@ class Listener(HubListener): assert message.subset.subset_state.left is data.id['x'] assert message.subset.subset_state.operator is operator.ge assert message.subset.subset_state.right == 3 + + +def test_message_once_all_subsets_exist(): + + # Make sure that when we create a new subset via EditSubsetMode, we don't + # get two separate messages for creation and updating, but instead just a + # single create message with the right subset state. + + app = Application() + + class Listener(HubListener): + pass + + listener = Listener() + + data1 = Data(x=[1, 2, 3, 4, 5]) + app.data_collection.append(data1) + + data2 = Data(x=[1, 2, 3, 4, 5]) + app.data_collection.append(data2) + + def handler(msg): + assert len(data1.subsets) == len(data2.subsets) + + app.session.hub.subscribe(listener, SubsetCreateMessage, handler=handler) + + cmd = ApplySubsetState(data_collection=app.data_collection, + subset_state=data1.id['x'] >= 3, + override_mode=ReplaceMode) + + app.session.command_stack.do(cmd) From 6559b181b3f9e45beca8ed699840919faf91a6c5 Mon Sep 17 00:00:00 2001 From: Thomas Robitaille Date: Mon, 6 Jan 2025 14:10:13 +0000 Subject: [PATCH 3/7] Ensure that callbacks are delayed until all subsets are added or deleted --- glue/core/data_collection.py | 23 ++++++++++++++--------- glue/core/tests/test_edit_subset_mode.py | 12 +++++++++++- 2 files changed, 25 insertions(+), 10 deletions(-) diff --git a/glue/core/data_collection.py b/glue/core/data_collection.py index 863a7baa8..d8ea2dfb6 100644 --- a/glue/core/data_collection.py +++ b/glue/core/data_collection.py @@ -269,9 +269,13 @@ def new_subset_group(self, label=None, subset_state=None, **kwargs): self._sg_count += 1 label = label or 'Subset %i' % self._sg_count - result = SubsetGroup(label=label, subset_state=subset_state, **kwargs) - self._subset_groups.append(result) - result.register(self) + + # We delay callbacks so that SubsetCreateMessages are only emitted once + # the subset exists in all datasets. + with self.hub.delay_callbacks(): + result = SubsetGroup(label=label, subset_state=subset_state, **kwargs) + self._subset_groups.append(result) + result.register(self) return result def remove_subset_group(self, subset_grp): @@ -281,12 +285,13 @@ def remove_subset_group(self, subset_grp): if subset_grp not in self._subset_groups: return - # remove from list first, so that group appears deleted - # by the time the first SubsetDelete message is broadcast - self._subset_groups.remove(subset_grp) - for s in subset_grp.subsets: - s.delete() - subset_grp.unregister(self.hub) + # We delay callbacks so that SubsetDelteMessages are only emitted once + # the subset exists in all datasets. + with self.hub.delay_callbacks(): + self._subset_groups.remove(subset_grp) + for s in subset_grp.subsets: + s.delete() + subset_grp.unregister(self.hub) def suggest_merge_label(self, *data): """ diff --git a/glue/core/tests/test_edit_subset_mode.py b/glue/core/tests/test_edit_subset_mode.py index db65bdc71..af2e04434 100644 --- a/glue/core/tests/test_edit_subset_mode.py +++ b/glue/core/tests/test_edit_subset_mode.py @@ -7,7 +7,7 @@ from ..application_base import Application from ..hub import HubListener -from ..message import SubsetCreateMessage, SubsetUpdateMessage +from ..message import SubsetCreateMessage, SubsetUpdateMessage, SubsetDeleteMessage from ..command import ApplySubsetState from ..data import Component, Data from ..data_collection import DataCollection @@ -152,13 +152,23 @@ class Listener(HubListener): data2 = Data(x=[1, 2, 3, 4, 5]) app.data_collection.append(data2) + count = [0] + def handler(msg): assert len(data1.subsets) == len(data2.subsets) + count[0] += 1 app.session.hub.subscribe(listener, SubsetCreateMessage, handler=handler) + app.session.hub.subscribe(listener, SubsetDeleteMessage, handler=handler) cmd = ApplySubsetState(data_collection=app.data_collection, subset_state=data1.id['x'] >= 3, override_mode=ReplaceMode) app.session.command_stack.do(cmd) + + assert count[0] == 2 + + app.data_collection.remove_subset_group(app.data_collection.subset_groups[0]) + + assert count[0] == 4 From 8fcce5a18e7c27d66cbcdb386ce6bdaf7e3618b8 Mon Sep 17 00:00:00 2001 From: Thomas Robitaille Date: Mon, 6 Jan 2025 14:12:57 +0000 Subject: [PATCH 4/7] Fix code style --- glue/core/data_collection.py | 1 - 1 file changed, 1 deletion(-) diff --git a/glue/core/data_collection.py b/glue/core/data_collection.py index d8ea2dfb6..95f21906f 100644 --- a/glue/core/data_collection.py +++ b/glue/core/data_collection.py @@ -269,7 +269,6 @@ def new_subset_group(self, label=None, subset_state=None, **kwargs): self._sg_count += 1 label = label or 'Subset %i' % self._sg_count - # We delay callbacks so that SubsetCreateMessages are only emitted once # the subset exists in all datasets. with self.hub.delay_callbacks(): From ec79441c02e09b8101a8016966db2eac9ae35f59 Mon Sep 17 00:00:00 2001 From: Thomas Robitaille Date: Mon, 6 Jan 2025 16:32:42 +0000 Subject: [PATCH 5/7] Update test_edit_subset_mode.py Co-authored-by: Derek Homeier --- glue/core/tests/test_edit_subset_mode.py | 39 ++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/glue/core/tests/test_edit_subset_mode.py b/glue/core/tests/test_edit_subset_mode.py index af2e04434..c0d9b4da4 100644 --- a/glue/core/tests/test_edit_subset_mode.py +++ b/glue/core/tests/test_edit_subset_mode.py @@ -133,6 +133,45 @@ class Listener(HubListener): assert message.subset.subset_state.right == 3 +def test_broadcast_to_collection(): + """A data collection input works on each data component""" + + handler = MagicMock() + + app = Application() + + class Listener(HubListener): + pass + + listener = Listener() + + app.session.hub.subscribe(listener, SubsetCreateMessage, handler=handler) + app.session.hub.subscribe(listener, SubsetUpdateMessage, handler=handler) + + data = Data(x=[1, 2, 3, 4, 5]) + data2 = Data(x=[2, 3, 4, 5, 6]) + + app.data_collection.append(data) + app.data_collection.append(data2) + + cmd = ApplySubsetState(data_collection=app.data_collection, + subset_state=data.id['x'] >= 3, + override_mode=ReplaceMode) + + app.session.command_stack.do(cmd) + + assert len(data2.subsets) == 1 + assert data2.subsets[0].label == 'Subset 1' + # fails with `IncompatibleAttribute` exception + # assert data2.get_subset_object(subset_id='Subset 1', cls=NDDataArray) + + assert handler.call_count == 2 + + assert data2.subsets[0].subset_state.left is data.id['x'] + assert data2.subsets[0].subset_state.operator is operator.ge + assert data2.subsets[0].subset_state.right == 3 + + def test_message_once_all_subsets_exist(): # Make sure that when we create a new subset via EditSubsetMode, we don't From a3aaca8fc6d86d0f94590f308f77ebcd1f78d5ca Mon Sep 17 00:00:00 2001 From: Derek Homeier Date: Mon, 6 Jan 2025 18:51:19 +0100 Subject: [PATCH 6/7] Test subset_state identity in all subsets --- glue/core/tests/test_edit_subset_mode.py | 1 + 1 file changed, 1 insertion(+) diff --git a/glue/core/tests/test_edit_subset_mode.py b/glue/core/tests/test_edit_subset_mode.py index c0d9b4da4..1249cf180 100644 --- a/glue/core/tests/test_edit_subset_mode.py +++ b/glue/core/tests/test_edit_subset_mode.py @@ -195,6 +195,7 @@ class Listener(HubListener): def handler(msg): assert len(data1.subsets) == len(data2.subsets) + assert data2.subsets[0].subset_state is data1.subsets[0].subset_state count[0] += 1 app.session.hub.subscribe(listener, SubsetCreateMessage, handler=handler) From 97ae3fd820ee8fd3434c67059dc713734e8c4f86 Mon Sep 17 00:00:00 2001 From: Derek Homeier Date: Wed, 8 Jan 2025 01:44:30 +0100 Subject: [PATCH 7/7] Fix subset_state test for remove_subset_group --- glue/core/tests/test_edit_subset_mode.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/glue/core/tests/test_edit_subset_mode.py b/glue/core/tests/test_edit_subset_mode.py index 1249cf180..3875ecb47 100644 --- a/glue/core/tests/test_edit_subset_mode.py +++ b/glue/core/tests/test_edit_subset_mode.py @@ -195,7 +195,8 @@ class Listener(HubListener): def handler(msg): assert len(data1.subsets) == len(data2.subsets) - assert data2.subsets[0].subset_state is data1.subsets[0].subset_state + for i in range(len(data1.subsets)): + assert data2.subsets[i].subset_state is data1.subsets[i].subset_state count[0] += 1 app.session.hub.subscribe(listener, SubsetCreateMessage, handler=handler)