From a9f9bbb151146df69a87ba99c8b8b08b84967b61 Mon Sep 17 00:00:00 2001 From: Thomas Robitaille Date: Fri, 23 Feb 2018 10:48:07 +0000 Subject: [PATCH 01/19] Added a regression test to make sure that removing a component removes any derived component that depends on it. --- glue/core/tests/test_data.py | 27 ++++++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) diff --git a/glue/core/tests/test_data.py b/glue/core/tests/test_data.py index 19b612006..4565fb844 100644 --- a/glue/core/tests/test_data.py +++ b/glue/core/tests/test_data.py @@ -370,6 +370,31 @@ def test_add_derived_implicit(self): # There should be five components: x, y, z, pixel, and world assert len(data.components) == 5 + def test_remove_derived_dependency(self): + + # Regression test for a bug that occurred when removing a component + # used in a derived component, which should also remove the derived + # component itself. + + data = Data(x=[1, 2, 3], y=[2, 3, 4], label='data1') + + data['z'] = data.id['x'] + 1 + + x_id = data.id['x'] + z_id = data.id['z'] + + # There should be five components: x, y, z, pixel, and world + assert len(data.components) == 5 + + data.remove_component(data.id['x']) + + # This should also remove z since it depends on x + + assert len(data.components) == 3 + + assert x_id not in data.components + assert z_id not in data.components + class TestROICreation(object): @@ -377,7 +402,7 @@ def test_range_roi(self): d = Data(xdata=[1, 2, 3], ydata=[1, 2, 3]) comp = d.get_component(d.id['xdata']) - roi = RangeROI('x', min=2,max=3) + roi = RangeROI('x', min=2, max=3) s = comp.subset_from_roi('xdata', roi) assert isinstance(s, RangeSubsetState) np.testing.assert_array_equal((s.lo, s.hi), From b97e4bbc8016b3e4276749a360871607dcb10ed3 Mon Sep 17 00:00:00 2001 From: Thomas Robitaille Date: Fri, 23 Feb 2018 10:49:50 +0000 Subject: [PATCH 02/19] Added regression test to make sure that removing a component from a dataset removes any links that depend on that component --- glue/core/tests/test_link_manager.py | 31 ++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/glue/core/tests/test_link_manager.py b/glue/core/tests/test_link_manager.py index caa5692a6..115c9346c 100644 --- a/glue/core/tests/test_link_manager.py +++ b/glue/core/tests/test_link_manager.py @@ -258,3 +258,34 @@ def test_remove_data_removes_links(self): # Removing dataset should remove related links dc.remove(d1) assert len(dc.links) == 2 + + def test_remove_component_removes_links(self): + + d1 = Data(x=[[1, 2], [3, 4]], label='image') + d2 = Data(a=[1, 2, 3], b=[4, 5, 6], label='catalog') + + dc = DataCollection([d1, d2]) + + assert len(dc.links) == 6 + + dc.add_link(LinkSame(d1.id['x'], d2.id['a'])) + + assert len(dc.links) == 7 + + assert len(d1.components) == 6 + assert len(d2.components) == 5 + + assert d1.id['x'] in d2.components + assert d2.id['a'] in d1.components + + # Removing component x from d1 should remove related links and + # derived components. + d1.remove_component(d2.id['a']) + + assert len(dc.links) == 6 + + assert len(d1.components) == 5 + assert len(d2.components) == 4 + + assert not any(cid.label == 'a' for cid in d1.components) + assert not any(cid.label == 'x' for cid in d2.components) From 0375e01a61621dff8e6d50a932f7f2a72aeebebe Mon Sep 17 00:00:00 2001 From: Thomas Robitaille Date: Fri, 23 Feb 2018 11:54:48 +0000 Subject: [PATCH 03/19] Modify Data so that it is responsible for getting rid of derived components that can no longer be derived. Also make it so that Data can contain 'externally derived components' that are separate from the main components - this container can then be used to store derived components from links. --- glue/core/data.py | 102 ++++++++++++++++++++++------------- glue/core/tests/test_data.py | 36 ++++++++----- 2 files changed, 88 insertions(+), 50 deletions(-) diff --git a/glue/core/data.py b/glue/core/data.py index e49c9f864..eb527b893 100644 --- a/glue/core/data.py +++ b/glue/core/data.py @@ -77,6 +77,7 @@ def __init__(self, label="", coords=None, **kwargs): # Components self._components = OrderedDict() + self._externally_derivable_components = OrderedDict() self._pixel_component_ids = ComponentIDList() self._world_component_ids = ComponentIDList() @@ -93,7 +94,7 @@ def __init__(self, label="", coords=None, **kwargs): self.style = VisualAttributes(parent=self) - self._coordinate_links = None + self._coordinate_links = [] self.data = self self.label = label @@ -179,14 +180,29 @@ def remove_component(self, component_id): :param component_id: the component to remove :type component_id: :class:`~glue.core.component_id.ComponentID` """ + # TODO: avoid too many messages when removing a component triggers + # the removal of derived components. if component_id in self._components: self._components.pop(component_id) + self._removed_derived_that_depend_on(component_id) if self.hub: msg = DataRemoveComponentMessage(self, component_id) self.hub.broadcast(msg) msg = ComponentsChangedMessage(self) self.hub.broadcast(msg) + def _removed_derived_that_depend_on(self, component_id): + """ + Remove internal derived components that can no longer be derived. + """ + remove = [] + for cid in self.derived_components: + comp = self.get_component(cid) + if component_id in comp.link.get_from_ids(): + remove.append(cid) + for cid in remove: + self.remove_component(cid) + @contract(other='isinstance(Data)', cid='cid_like', cid_other='cid_like') @@ -429,6 +445,19 @@ def add_component(self, component, label, hidden=False): return component_id + def _set_externally_derivable_components(self, derivable_components): + """ + Externally deriable components are components identified by component + IDs from other datasets. + + This method is meant for internal use only and is called by the link + manager. The ``derivable_components`` argument should be set to a + dictionary where the keys are the derivable component IDs, and the + values are DerivedComponent instances which can be used to get the + data. + """ + self._externally_derivable_components = derivable_components + @contract(link=ComponentLink, label='cid_like|None', returns=DerivedComponent) @@ -465,12 +494,40 @@ def _create_pixel_and_world_components(self, ndim): cid = PixelComponentID(i, "Pixel Axis %s" % label, hidden=True, parent=self) self.add_component(comp, cid) self._pixel_component_ids.append(cid) + if self.coords: for i in range(ndim): comp = CoordinateComponent(self, i, world=True) label = self.coords.axis_label(i) cid = self.add_component(comp, label, hidden=True) self._world_component_ids.append(cid) + self._set_up_coordinate_component_links(ndim) + + def _set_up_coordinate_component_links(self, ndim): + + def make_toworld_func(i): + def pix2world(*args): + return self.coords.pixel2world_single_axis(*args[::-1], axis=ndim - 1 - i) + return pix2world + + def make_topixel_func(i): + def world2pix(*args): + return self.coords.world2pixel_single_axis(*args[::-1], axis=ndim - 1 - i) + return world2pix + + result = [] + for i in range(ndim): + link = CoordinateComponentLink(self._pixel_component_ids, + self._world_component_ids[i], + self.coords, i) + result.append(link) + link = CoordinateComponentLink(self._world_component_ids, + self._pixel_component_ids[i], + self.coords, i, pixel2world=False) + result.append(link) + + self._coordinate_links = result + return result @property def components(self): @@ -545,7 +602,7 @@ def find_component_id(self, label): one takes precedence. """ - for cid_set in (self.primary_components, self.derived_components): + for cid_set in (self.primary_components, self.derived_components, list(self._externally_derivable_components)): result = [] for cid in cid_set: @@ -568,40 +625,7 @@ def coordinate_links(self): world. If no coordinate transformation object is present, return an empty list. """ - if self._coordinate_links: - return self._coordinate_links - - if not self.coords: - return [] - - if self.ndim != len(self._pixel_component_ids) or \ - self.ndim != len(self._world_component_ids): - # haven't populated pixel, world coordinates yet - return [] - - def make_toworld_func(i): - def pix2world(*args): - return self.coords.pixel2world_single_axis(*args[::-1], axis=self.ndim - 1 - i) - return pix2world - - def make_topixel_func(i): - def world2pix(*args): - return self.coords.world2pixel_single_axis(*args[::-1], axis=self.ndim - 1 - i) - return world2pix - - result = [] - for i in range(self.ndim): - link = CoordinateComponentLink(self._pixel_component_ids, - self._world_component_ids[i], - self.coords, i) - result.append(link) - link = CoordinateComponentLink(self._world_component_ids, - self._pixel_component_ids[i], - self.coords, i, pixel2world=False) - result.append(link) - - self._coordinate_links = result - return result + return self._coordinate_links @contract(axis=int, returns=ComponentID) def get_pixel_component_id(self, axis): @@ -822,9 +846,11 @@ def __getitem__(self, key): if isinstance(key, ComponentLink): return key.compute(self, view) - try: + if key in self._components: comp = self._components[key] - except KeyError: + elif key in self._externally_derivable_components: + comp = self._externally_derivable_components[key] + else: raise IncompatibleAttribute(key) shp = view_shape(self.shape, view) diff --git a/glue/core/tests/test_data.py b/glue/core/tests/test_data.py index 4565fb844..0f4ddb2ec 100644 --- a/glue/core/tests/test_data.py +++ b/glue/core/tests/test_data.py @@ -374,26 +374,38 @@ def test_remove_derived_dependency(self): # Regression test for a bug that occurred when removing a component # used in a derived component, which should also remove the derived - # component itself. + # component itself. To make things more fun, we set up a chain of + # derived components to make sure they are all removed. - data = Data(x=[1, 2, 3], y=[2, 3, 4], label='data1') + data = Data(a=[1, 2, 3], b=[2, 3, 4], label='data1') - data['z'] = data.id['x'] + 1 + data['c'] = data.id['a'] + 1 + data['d'] = data.id['c'] + 1 + data['e'] = data.id['d'] + 1 + data['f'] = data.id['e'] + 1 - x_id = data.id['x'] - z_id = data.id['z'] + a_id = data.id['a'] + b_id = data.id['b'] + c_id = data.id['c'] + d_id = data.id['d'] + e_id = data.id['e'] + f_id = data.id['f'] - # There should be five components: x, y, z, pixel, and world - assert len(data.components) == 5 + # There should be five components: pixel, world, a, b, c, d, e, f + assert len(data.components) == 8 - data.remove_component(data.id['x']) + data.remove_component(data.id['d']) - # This should also remove z since it depends on x + # This should also remove e and f since they depend on d - assert len(data.components) == 3 + assert len(data.components) == 5 - assert x_id not in data.components - assert z_id not in data.components + assert a_id in data.components + assert b_id in data.components + assert c_id in data.components + assert d_id not in data.components + assert e_id not in data.components + assert f_id not in data.components class TestROICreation(object): From 2e085616d123da5e9252873674af7238602ceff8 Mon Sep 17 00:00:00 2001 From: Thomas Robitaille Date: Fri, 23 Feb 2018 12:07:12 +0000 Subject: [PATCH 04/19] Added a Data.links property that can be used to get all the links internal to the dataset --- glue/core/data.py | 21 ++++++++++++++++++--- glue/core/tests/test_data.py | 19 +++++++++++++++++-- 2 files changed, 35 insertions(+), 5 deletions(-) diff --git a/glue/core/data.py b/glue/core/data.py index eb527b893..0b215077e 100644 --- a/glue/core/data.py +++ b/glue/core/data.py @@ -619,14 +619,29 @@ def find_component_id(self, label): return None return None + @property + def links(self): + """ + A list of all the links internal to the dataset. + """ + return self.coordinate_links + self.derived_links + @property def coordinate_links(self): - """A list of the ComponentLinks that connect pixel and - world. If no coordinate transformation object is present, - return an empty list. + """ + A list of the ComponentLinks that connect pixel and world. If no + coordinate transformation object is present, return an empty list. """ return self._coordinate_links + @property + def derived_links(self): + """ + A list of the links present inside all of the DerivedComponent objects + in this dataset. + """ + return [self.get_component(cid).link for cid in self.derived_components] + @contract(axis=int, returns=ComponentID) def get_pixel_component_id(self, axis): """Return the pixel :class:`glue.core.component_id.ComponentID` associated with a given axis diff --git a/glue/core/tests/test_data.py b/glue/core/tests/test_data.py index 0f4ddb2ec..72386e299 100644 --- a/glue/core/tests/test_data.py +++ b/glue/core/tests/test_data.py @@ -11,7 +11,7 @@ from ..component import Component, DerivedComponent, CategoricalComponent, DateTimeComponent from ..component_id import ComponentID -from ..component_link import ComponentLink +from ..component_link import ComponentLink, CoordinateComponentLink, BinaryComponentLink from ..coordinates import Coordinates from ..data import Data, pixel_label from ..exceptions import IncompatibleAttribute @@ -25,6 +25,7 @@ from .test_state import clone + class _TestCoordinates(Coordinates): def pixel2world(self, *args): @@ -407,6 +408,20 @@ def test_remove_derived_dependency(self): assert e_id not in data.components assert f_id not in data.components + def test_links_property(self): + + data = Data(a=[1, 2, 3], b=[2, 3, 4], label='data1') + + assert len(data.links) == 2 + assert isinstance(data.links[0], CoordinateComponentLink) + assert isinstance(data.links[1], CoordinateComponentLink) + + data['c'] = data.id['a'] + 1 + + assert len(data.links) == 3 + + assert isinstance(data.links[2], BinaryComponentLink) + class TestROICreation(object): @@ -420,7 +435,7 @@ def test_range_roi(self): np.testing.assert_array_equal((s.lo, s.hi), [2, 3]) - roi = RangeROI('y', min=2,max=3) + roi = RangeROI('y', min=2, max=3) s = comp.subset_from_roi('xdata', roi, other_att='ydata', other_comp=d.get_component(d.id['ydata'])) assert isinstance(s, RangeSubsetState) From c0fd0d90efaa02d3368db6c955b7a3d6c362c35e Mon Sep 17 00:00:00 2001 From: Thomas Robitaille Date: Fri, 23 Feb 2018 13:13:02 +0000 Subject: [PATCH 05/19] Refactor how data collection and link manager handle links Data objects are now responsible for storing internal derived component links and coordinate links, and the LinkManager fetches those links on-the-fly, removing the need to sync. --- glue/core/data.py | 8 ++ glue/core/data_collection.py | 51 +++------- glue/core/link_manager.py | 125 +++++++++++++----------- glue/core/tests/test_component_link.py | 27 ----- glue/core/tests/test_data_collection.py | 40 +++++--- glue/core/tests/test_link_manager.py | 51 ++++++---- glue/core/tests/test_links.py | 2 +- 7 files changed, 141 insertions(+), 163 deletions(-) diff --git a/glue/core/data.py b/glue/core/data.py index 0b215077e..2a88c669e 100644 --- a/glue/core/data.py +++ b/glue/core/data.py @@ -481,6 +481,10 @@ def add_component_link(self, link, label=None, hidden=False): raise TypeError("Cannot add component_link: " "has no 'to' ComponentID") + for cid in link.get_from_ids(): + if cid not in self.components: + raise ValueError("Can only add internal links with add_component_link - use DataCollection.add_link to add inter-data links") + dc = DerivedComponent(self, link) to_ = link.get_to_id() self.add_component(dc, label=to_, hidden=hidden) @@ -537,6 +541,10 @@ def components(self): """ return list(self._components.keys()) + @property + def externally_derivable_components(self): + return list(self._externally_derivable_components.keys()) + @property def visible_components(self): """ :class:`ComponentIDs ` for all non-hidden components. diff --git a/glue/core/data_collection.py b/glue/core/data_collection.py index bccb9ecbe..51d108088 100644 --- a/glue/core/data_collection.py +++ b/glue/core/data_collection.py @@ -37,7 +37,8 @@ def __init__(self, data=None): :param data: :class:`~glue.core.data.Data` object, or list of such objects """ super(DataCollection, self).__init__() - self._link_manager = LinkManager() + + self._link_manager = LinkManager(self) self._data = [] self.hub = None @@ -76,6 +77,7 @@ def append(self, data): s.register() msg = DataCollectionAddMessage(self, data) self.hub.broadcast(msg) + self._sync_link_manager() def extend(self, data): @@ -117,16 +119,7 @@ def _sync_link_manager(self): # Avoid circular calls with self._no_sync_link_manager(): - - # add any links in the data - for d in self._data: - for derived in d.derived_components: - self._link_manager.add_link(d.get_component(derived).link) - for link in d.coordinate_links: - self._link_manager.add_link(link) - - for d in self._data: - self._link_manager.update_data_components(d) + self._link_manager.update_externally_derivable_components() @contextmanager def _no_sync_link_manager(self): @@ -151,13 +144,7 @@ def add_link(self, links): :class:`~glue.core.component_link.ComponentLink` instances, or a :class:`~glue.core.link_helpers.LinkCollection` """ - # Adding derived comments can trigger _sync_link_manager but we - # don't need to do this since we are explicitly calling - # update_data_components here. - with self._no_sync_link_manager(): - self._link_manager.add_link(links) - for d in self._data: - self._link_manager.update_data_components(d) + self._link_manager.add_link(links) def remove_link(self, links): """ @@ -170,38 +157,22 @@ def remove_link(self, links): :class:`~glue.core.component_link.ComponentLink` instances, or a :class:`~glue.core.link_helpers.LinkCollection` """ - # Removing derived comments can trigger _sync_link_manager but we - # don't need to do this since we are explicitly calling - # update_data_components here. - with self._no_sync_link_manager(): - self._link_manager.remove_link(links) - for d in self._data: - self._link_manager.update_data_components(d) + self._link_manager.remove_link(links) def _merge_link(self, link): pass def set_links(self, links): - """Override the links in the collection, and update data - objects as necessary. + """ + Override the links in the collection, and update data objects as + necessary. :param links: The new links. An iterable of :class:`~glue.core.component_link.ComponentLink` instances """ + self._link_manager.clear_links() + self._link_manager.add_link(links) - # Adding derived comments can trigger _sync_link_manager but we - # don't need to do this since we are explicitly calling - # update_data_components here. - - with self._no_sync_link_manager(): - - self._link_manager.clear() - - for link in links: - self._link_manager.add_link(link) - - for d in self._data: - self._link_manager.update_data_components(d) def register_to_hub(self, hub): """ Register managed data objects to a hub. diff --git a/glue/core/link_manager.py b/glue/core/link_manager.py index 07ef12580..72580c30c 100644 --- a/glue/core/link_manager.py +++ b/glue/core/link_manager.py @@ -21,7 +21,7 @@ from glue.external import six from glue.core.hub import HubListener -from glue.core.message import DataCollectionDeleteMessage, ComponentsChangedMessage, DataAddComponentMessage, DataRemoveComponentMessage +from glue.core.message import DataCollectionDeleteMessage, DataRemoveComponentMessage from glue.core.contracts import contract from glue.core.link_helpers import LinkCollection from glue.core.component_link import ComponentLink @@ -131,18 +131,34 @@ class LinkManager(HubListener): and compute which components are accesible from which data sets """ - def __init__(self): - self._links = set() + def __init__(self, data_collection=None): + self._external_links = set() self.hub = None + self.trigger = False + self.data_collection = data_collection def register_to_hub(self, hub): self.hub = hub + self.hub.subscribe(self, DataRemoveComponentMessage, + handler=self._component_removed) self.hub.subscribe(self, DataCollectionDeleteMessage, handler=self._data_removed) + def _component_removed(self, msg): + remove = [] + remove_cid = msg.component_id + for link in self._external_links: + ids = set(link.get_from_ids()) | set([link.get_to_id()]) + for cid in ids: + if cid is remove_cid: + remove.append(link) + break + for link in remove: + self.remove_link(link) + def _data_removed(self, msg): remove = [] - for link in self._links: + for link in self._external_links: ids = set(link.get_from_ids()) | set([link.get_to_id()]) for cid in ids: if cid.parent is msg.data: @@ -151,7 +167,10 @@ def _data_removed(self, msg): for link in remove: self.remove_link(link) - def add_link(self, link): + def clear_links(self): + self._external_links.clear() + + def add_link(self, link, update_external=True): """ Ingest one or more ComponentLinks to the manager @@ -162,24 +181,33 @@ def add_link(self, link): """ if isinstance(link, (LinkCollection, list)): for l in link: - self.add_link(l) + self.add_link(l, update_external=False) + if update_external: + self.update_externally_derivable_components() else: - if link.inverse not in self._links: - self._links.add(link) + if link.inverse not in self._external_links: + self._external_links.add(link) + if update_external: + self.update_externally_derivable_components() @contract(link=ComponentLink) - def remove_link(self, link): + def remove_link(self, link, update_external=True): if isinstance(link, (LinkCollection, list)): for l in link: - self.remove_link(l) + self.remove_link(l, update_exernal=False) + if update_external: + self.update_externally_derivable_components() else: logging.getLogger(__name__).debug('removing link %s', link) - self._links.remove(link) + self._external_links.remove(link) + if update_external: + self.update_externally_derivable_components() @contract(data=Data) - def update_data_components(self, data): - """Update all the DerivedComponents in a data object, based on - all the Components deriveable based on the links in self. + def update_externally_derivable_components(self, data=None): + """ + Update all the externally derived components in all data objects, based + on all the Components deriveable based on the links in self. This overrides any ComponentLinks stored in the DerivedComponents of the data itself -- any components which @@ -195,62 +223,41 @@ def update_data_components(self, data): DerivedComponents will be replaced / added into the data object """ - if self.hub is None: - self._remove_underiveable_components(data) - self._add_deriveable_components(data) + + if self.data_collection is None: + if data is None: + return + else: + data_collection = [data] else: - before = data.components[:] - with self.hub.ignore_callbacks(DataRemoveComponentMessage): - with self.hub.ignore_callbacks(DataAddComponentMessage): - with self.hub.ignore_callbacks(ComponentsChangedMessage): - self._remove_underiveable_components(data) - self._add_deriveable_components(data) - after = data.components[:] - if len(before) != len(after) or any(before[i] is not after[i] for i in range(len(before))): - msg = ComponentsChangedMessage(data) - self.hub.broadcast(msg) + data_collection = self.data_collection - @property - def _inverse_links(self): - return set(link.inverse for link in self._links if link.inverse is not None) + for data in data_collection: + links = discover_links(data, self._links | self._inverse_links) + comps = {} + for cid, link in six.iteritems(links): + d = DerivedComponent(data, link) + comps[cid] = d + data._set_externally_derivable_components(comps) - def _remove_underiveable_components(self, data): - """ Find and remove any DerivedComponent in the data - which requires a ComponentLink not tracked by this LinkManager - """ - data_links = set(data.get_component(dc).link - for dc in data.derived_components) - missing_links = data_links - self._links - self._inverse_links - to_remove = [] - for m in missing_links: - to_remove.extend(find_dependents(data, m)) - - if getattr(data, 'hub', None) is None: - for r in to_remove: - data.remove_component(r) + @property + def _links(self): + if self.data_collection is None: + data_links = set() else: - with data.hub.delay_callbacks(): - for r in to_remove: - data.remove_component(r) + data_links = set(link for data in self.data_collection for link in data.links) + return data_links | self._external_links - def _add_deriveable_components(self, data): - """Find and add any DerivedComponents that a data object can - calculate given the ComponentLinks tracked by this - LinkManager - - """ - links = discover_links(data, self._links | self._inverse_links) - for cid, link in six.iteritems(links): - d = DerivedComponent(data, link) - if cid not in data.components: - data.add_component(d, cid) + @property + def _inverse_links(self): + return set(link.inverse for link in self._links if link.inverse is not None) @property def links(self): return list(self._links) def clear(self): - self._links.clear() + self._external_links.clear() def __contains__(self, item): return item in self._links diff --git a/glue/core/tests/test_component_link.py b/glue/core/tests/test_component_link.py index 01b302717..6e0eca7e6 100644 --- a/glue/core/tests/test_component_link.py +++ b/glue/core/tests/test_component_link.py @@ -293,30 +293,3 @@ def test_link_str(): assert str(x + x + y) == ('((x + x) + y)') assert repr(x + y) == '' - - -def test_duplicated_links_remove_first_input(): - """ - # test changes introduced for #508 - """ - - d1 = Data(x=[1, 2, 3]) - d2 = Data(y=[2, 4, 6]) - - x = d1.id['x'] - y = d2.id['y'] - - dc = DataCollection([d1, d2]) - - dc.add_link(LinkSame(x, y)) - assert x in d2.components - assert x in d2.components - - # NOTE: the behavior tested here is not desirable anymore, so the relevant - # parts that are no longer true have been commented out and replaced - # by the new behavior. - - # assert y not in d2.components - # assert y not in d1.components - assert y in d2.components - assert y in d1.components diff --git a/glue/core/tests/test_data_collection.py b/glue/core/tests/test_data_collection.py index 2ea2b958a..20f7fae95 100644 --- a/glue/core/tests/test_data_collection.py +++ b/glue/core/tests/test_data_collection.py @@ -5,7 +5,7 @@ import pytest import numpy as np from mock import MagicMock -from numpy.testing import assert_array_equal +from numpy.testing import assert_array_equal, assert_equal from ..coordinates import Coordinates from ..component_link import ComponentLink @@ -15,6 +15,7 @@ from ..message import (Message, DataCollectionAddMessage, DataRemoveComponentMessage, DataCollectionDeleteMessage, ComponentsChangedMessage) +from ..exceptions import IncompatibleAttribute class HubLog(HubListener): @@ -126,8 +127,10 @@ def test_len(self): assert len(self.dc) == 0 def test_derived_links_autoadd(self): - """When appending a data set, its DerivedComponents - should be ingested into the LinkManager""" + """ + When appending a data set, its DerivedComponents should be ingested into + the LinkManager + """ d = Data() id1 = ComponentID("id1") id2 = ComponentID("id2") @@ -154,18 +157,18 @@ def test_catch_data_add_component_message(self): self.dc.append(d) d.add_component(Component(np.array([1, 2, 3])), id1) - assert not link in self.dc._link_manager + assert link not in self.dc._link_manager d.add_component(dc, id2) msg = self.log.messages[-1] assert isinstance(msg, ComponentsChangedMessage) assert link in self.dc._link_manager - def test_coordinate_links_auto_added(self): + def test_links_auto_added(self): id1 = ComponentID("id1") id2 = ComponentID("id2") link = ComponentLink([id1], id2) - self.data.coordinate_links = [link] + self.data.links = [link] self.dc.append(self.data) assert link in self.dc.links @@ -178,17 +181,18 @@ def test_add_links(self): assert link in self.dc.links def test_add_links_updates_components(self): - """setting links attribute automatically adds components to data""" + """ + Setting links attribute automatically adds components to data + """ d = Data() comp = Component(np.array([1, 2, 3])) id1 = ComponentID("id1") d.add_component(comp, id1) id2 = ComponentID("id2") self.dc.append(d) - link = ComponentLink([id1], id2, using=lambda x: None) - + link = ComponentLink([id1], id2) self.dc.set_links([link]) - assert id2 in d.components + assert_equal(d[id2], d[id1]) def test_links_propagated(self): """Web of links is grown and applied to data automatically""" @@ -200,19 +204,23 @@ def test_links_propagated(self): cid2 = ComponentID('b') cid3 = ComponentID('c') - links1 = ComponentLink([cid1], cid2, lambda x: None) + links1 = ComponentLink([cid1], cid2) dc.add_link(links1) - assert cid2 in d.components - links2 = ComponentLink([cid2], cid3, lambda x: None) + assert_equal(d[cid2], d[cid1]) + + links2 = ComponentLink([cid2], cid3) dc.add_link(links2) - assert cid3 in d.components + assert_equal(d[cid3], d[cid2]) dc.remove_link(links2) - assert cid3 not in d.components + with pytest.raises(IncompatibleAttribute): + d[cid3] + assert_equal(d[cid2], d[cid1]) dc.remove_link(links1) - assert cid2 not in d.components + with pytest.raises(IncompatibleAttribute): + d[cid2] def test_merge_links(self): """Trivial links should be merged, discarding the duplicate ID""" diff --git a/glue/core/tests/test_link_manager.py b/glue/core/tests/test_link_manager.py index 115c9346c..efbe31354 100644 --- a/glue/core/tests/test_link_manager.py +++ b/glue/core/tests/test_link_manager.py @@ -50,7 +50,8 @@ def example_components(self, add_derived=True): self.primary = [c1, c2] self.direct = [c3, c4] - self.derived = [c5, c6] + self.derived = [] + self.externally_derived = [c5, c6] self.inaccessible = [c7, c8] @@ -160,32 +161,33 @@ def test_setup(self): expected = set() assert set(self.data.derived_components) == expected - def test_update_data_components_adds_correctly(self): + def test_update_externally_derivable_components_adds_correctly(self): example_components(self, add_derived=False) lm = LinkManager() list(map(lm.add_link, self.links)) - lm.update_data_components(self.data) - derived = set(self.data.derived_components) - expected = set(self.derived + self.direct) + lm.update_externally_derivable_components(self.data) + derived = set(self.data.externally_derivable_components) + expected = set(self.externally_derived + self.direct) assert derived == expected - def test_update_data_components_removes_correctly(self): + def test_update_externally_derivable_components_removes_correctly(self): # add all but last link to manager example_components(self, add_derived=False) lm = LinkManager() list(map(lm.add_link, self.links[:-1])) + lm.update_externally_derivable_components(self.data) # manually add last link as derived component dc = DerivedComponent(self.data, self.links[-1]) - self.data.add_component(dc, dc.link.get_to_id()) + self.data._externally_derivable_components.update({dc.link.get_to_id(): dc}) removed = set([dc.link.get_to_id()]) - assert dc.link.get_to_id() in self.data.derived_components + assert dc.link.get_to_id() in self.data.externally_derivable_components # this link should be removed upon update_components - lm.update_data_components(self.data) - derived = set(self.data.derived_components) - expected = set(self.direct + self.derived) - removed + lm.update_externally_derivable_components(self.data) + derived = set(self.data.externally_derivable_components) + expected = set(self.direct + self.externally_derived) - removed assert derived == expected def test_derived_links_correctwith_mergers(self): @@ -268,24 +270,33 @@ def test_remove_component_removes_links(self): assert len(dc.links) == 6 + assert len(d1.components) == 5 + assert len(d2.components) == 4 + + assert len(d1.externally_derivable_components) == 0 + assert len(d2.externally_derivable_components) == 0 + dc.add_link(LinkSame(d1.id['x'], d2.id['a'])) assert len(dc.links) == 7 - assert len(d1.components) == 6 - assert len(d2.components) == 5 + assert len(d1.components) == 5 + assert len(d2.components) == 4 - assert d1.id['x'] in d2.components - assert d2.id['a'] in d1.components + assert len(d1.externally_derivable_components) == 1 + assert len(d2.externally_derivable_components) == 1 - # Removing component x from d1 should remove related links and + assert d1.id['x'] in d2.externally_derivable_components + assert d2.id['a'] in d1.externally_derivable_components + + # Removing component a from d2 should remove related links and # derived components. - d1.remove_component(d2.id['a']) + d2.remove_component(d2.id['a']) assert len(dc.links) == 6 assert len(d1.components) == 5 - assert len(d2.components) == 4 + assert len(d2.components) == 3 - assert not any(cid.label == 'a' for cid in d1.components) - assert not any(cid.label == 'x' for cid in d2.components) + assert len(d1.externally_derivable_components) == 0 + assert len(d2.externally_derivable_components) == 0 diff --git a/glue/core/tests/test_links.py b/glue/core/tests/test_links.py index 24a06bd2a..191069007 100644 --- a/glue/core/tests/test_links.py +++ b/glue/core/tests/test_links.py @@ -18,7 +18,7 @@ def test_1d_world_link(): dc.add_link(LinkSame(d2.get_world_component_id(0), d1.id['x'])) - assert d2.get_world_component_id(0) in d1.components + assert d2.get_world_component_id(0) in d1.externally_derivable_components np.testing.assert_array_equal(d1[d2.get_world_component_id(0)], x) np.testing.assert_array_equal(d1[d2.get_pixel_component_id(0)], x) From 865f6ba45e4a010982ed418f22c908929c2ef884 Mon Sep 17 00:00:00 2001 From: Thomas Robitaille Date: Fri, 23 Feb 2018 13:15:49 +0000 Subject: [PATCH 06/19] Make sure that component manager works properly when links are present --- .../component_manager/qt/tests/test_component_manager.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/glue/dialogs/component_manager/qt/tests/test_component_manager.py b/glue/dialogs/component_manager/qt/tests/test_component_manager.py index ae387f577..79733ed49 100644 --- a/glue/dialogs/component_manager/qt/tests/test_component_manager.py +++ b/glue/dialogs/component_manager/qt/tests/test_component_manager.py @@ -7,6 +7,7 @@ from glue.core import Data, DataCollection, HubListener, ComponentID from glue.core import message as msg from glue.utils.qt import get_qapp +from glue.core.component_link import ComponentLink from glue.core.parse import ParsedCommand, ParsedComponentLink from ..component_manager import ComponentManagerWidget, EquationEditorDialog @@ -86,14 +87,16 @@ def assert_exact_changes(self, added=[], removed=[], renamed=[], reordered=[], n assert set(added) == set(self.added) assert set(removed) == set(self.removed) assert set(renamed) == set(self.renamed) - assert set(reordered) == set(self.reordered) + assert reordered == self.reordered assert numerical is self.numerical class TestComponentManagerWidget: def setup_method(self): + self.app = get_qapp() + self.data1 = Data(x=[1, 2, 3], y=[3.5, 4.5, -1.0], z=['a', 'r', 'w']) self.data2 = Data(a=[3, 4, 1], b=[1.5, -2.0, 3.5], c=['y', 'e', 'r']) @@ -104,6 +107,10 @@ def setup_method(self): self.data2.add_component_link(link) self.data_collection = DataCollection([self.data1, self.data2]) + + link = ComponentLink([self.data1.id['x']], self.data2.id['a']) + self.data_collection.add_link(link) + self.listener1 = ChangeListener(self.data1) self.listener2 = ChangeListener(self.data2) From afa3c450dc376eab40b2eeed69269ee0c5f3b9b3 Mon Sep 17 00:00:00 2001 From: Thomas Robitaille Date: Fri, 23 Feb 2018 16:13:35 +0000 Subject: [PATCH 07/19] Fix tests --- glue/core/data.py | 12 +++++++++--- glue/core/link_manager.py | 2 +- glue/core/message.py | 4 ++++ glue/core/tests/test_data_collection.py | 9 +++++++-- .../component_manager/qt/component_manager.py | 14 +++++++++++++- glue/viewers/common/qt/data_viewer_with_state.py | 4 ++++ glue/viewers/image/layer_artist.py | 6 +++++- 7 files changed, 43 insertions(+), 8 deletions(-) diff --git a/glue/core/data.py b/glue/core/data.py index 2a88c669e..3a5fe7aff 100644 --- a/glue/core/data.py +++ b/glue/core/data.py @@ -10,7 +10,8 @@ from glue.core.message import (DataUpdateMessage, DataRemoveComponentMessage, DataAddComponentMessage, NumericalDataChangedMessage, SubsetCreateMessage, ComponentsChangedMessage, - ComponentReplacedMessage, DataReorderComponentMessage) + ComponentReplacedMessage, DataReorderComponentMessage, + ExternallyDerivableComponentsChangedMessage) from glue.core.decorators import clear_cache from glue.core.util import split_component_view from glue.core.hub import Hub @@ -457,6 +458,9 @@ def _set_externally_derivable_components(self, derivable_components): data. """ self._externally_derivable_components = derivable_components + if self.hub: + msg = ExternallyDerivableComponentsChangedMessage(self) + self.hub.broadcast(msg) @contract(link=ComponentLink, label='cid_like|None', @@ -908,9 +912,11 @@ def get_component(self, component_id): if isinstance(component_id, six.string_types): component_id = self.id[component_id] - try: + if component_id in self._components: return self._components[component_id] - except KeyError: + elif component_id in self._externally_derivable_components: + return self._externally_derivable_components[component_id] + else: raise IncompatibleAttribute(component_id) def to_dataframe(self, index=None): diff --git a/glue/core/link_manager.py b/glue/core/link_manager.py index 72580c30c..a8d5c5f6e 100644 --- a/glue/core/link_manager.py +++ b/glue/core/link_manager.py @@ -194,7 +194,7 @@ def add_link(self, link, update_external=True): def remove_link(self, link, update_external=True): if isinstance(link, (LinkCollection, list)): for l in link: - self.remove_link(l, update_exernal=False) + self.remove_link(l, update_external=False) if update_external: self.update_externally_derivable_components() else: diff --git a/glue/core/message.py b/glue/core/message.py index 3bbda4141..c638f631d 100644 --- a/glue/core/message.py +++ b/glue/core/message.py @@ -156,6 +156,10 @@ def __init__(self, sender, component_ids, tag=None): self.component_ids = component_ids +class ExternallyDerivableComponentsChangedMessage(DataMessage): + pass + + class ComponentsChangedMessage(DataMessage): pass diff --git a/glue/core/tests/test_data_collection.py b/glue/core/tests/test_data_collection.py index 20f7fae95..d1a45387b 100644 --- a/glue/core/tests/test_data_collection.py +++ b/glue/core/tests/test_data_collection.py @@ -14,7 +14,7 @@ from ..hub import HubListener from ..message import (Message, DataCollectionAddMessage, DataRemoveComponentMessage, DataCollectionDeleteMessage, - ComponentsChangedMessage) + ComponentsChangedMessage, ExternallyDerivableComponentsChangedMessage) from ..exceptions import IncompatibleAttribute @@ -393,9 +393,14 @@ def test_remove_component_message(self): data.remove_component(remove_id) - msg = self.log.messages[-2] + print(self.log.messages) + + msg = self.log.messages[-3] assert isinstance(msg, DataRemoveComponentMessage) assert msg.component_id is remove_id + msg = self.log.messages[-2] + assert isinstance(msg, ExternallyDerivableComponentsChangedMessage) + msg = self.log.messages[-1] assert isinstance(msg, ComponentsChangedMessage) diff --git a/glue/dialogs/component_manager/qt/component_manager.py b/glue/dialogs/component_manager/qt/component_manager.py index 5088b3ebe..be53cb975 100644 --- a/glue/dialogs/component_manager/qt/component_manager.py +++ b/glue/dialogs/component_manager/qt/component_manager.py @@ -108,6 +108,17 @@ def __init__(self, data_collection=None, parent=None): self._state[data][cid] = comp_state self._components[data]['derived'].append(cid) + self._components[data]['other'] = [] + + handled_components = (data.pixel_component_ids + + data.world_component_ids + + self._components[data]['main'] + + self._components[data]['derived']) + + for cid in data.components: + if cid not in handled_components: + self._components[data]['other'].append(cid) + # Populate data combo for data in self.data_collection: self.ui.combosel_data.addItem(data.label, userData=data) @@ -302,6 +313,7 @@ def accept(self): cids_main = self._components[data]['main'] cids_derived = self._components[data]['derived'] + cids_other = self._components[data]['other'] # First deal with renaming of components for cid_new in cids_main + cids_derived: @@ -309,7 +321,7 @@ def accept(self): if label != cid_new.label: cid_new.label = label - cids_all = data.pixel_component_ids + data.world_component_ids + cids_main + cids_derived + cids_all = data.pixel_component_ids + data.world_component_ids + cids_main + cids_derived + cids_other cids_existing = data.components diff --git a/glue/viewers/common/qt/data_viewer_with_state.py b/glue/viewers/common/qt/data_viewer_with_state.py index ea2acc511..78864a4ed 100644 --- a/glue/viewers/common/qt/data_viewer_with_state.py +++ b/glue/viewers/common/qt/data_viewer_with_state.py @@ -201,6 +201,10 @@ def register_to_hub(self, hub): handler=self._update_data, filter=self._has_data_or_subset) + hub.subscribe(self, msg.ExternallyDerivableComponentsChangedMessage, + handler=self._update_data, + filter=self._has_data_or_subset) + hub.subscribe(self, msg.SettingsChangeMessage, self._update_appearance_from_settings, filter=self._is_appearance_settings) diff --git a/glue/viewers/image/layer_artist.py b/glue/viewers/image/layer_artist.py index d2bc2b7b2..05de7dcd5 100644 --- a/glue/viewers/image/layer_artist.py +++ b/glue/viewers/image/layer_artist.py @@ -14,7 +14,7 @@ from glue.utils import color2rgb from glue.core.link_manager import is_equivalent_cid from glue.core import Data, HubListener -from glue.core.message import ComponentsChangedMessage +from glue.core.message import ComponentsChangedMessage, ExternallyDerivableComponentsChangedMessage from glue.external.modest_image import imshow @@ -36,6 +36,10 @@ def __init__(self, axes, viewer_state, layer_state=None, layer=None): handler=self._update_compatibility, filter=self._is_data_object) + self.layer.hub.subscribe(self, ExternallyDerivableComponentsChangedMessage, + handler=self._update_compatibility, + filter=self._is_data_object) + self._update_compatibility() def _is_data_object(self, message): From 8fbba3785e3b31dc2bd3f1dbac55144aab335052 Mon Sep 17 00:00:00 2001 From: Thomas Robitaille Date: Fri, 23 Feb 2018 18:19:45 +0000 Subject: [PATCH 08/19] Don't use lambda function to avoid issues with saving/reloading sessions --- glue/core/component_link.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/glue/core/component_link.py b/glue/core/component_link.py index 9e98ebbdb..3cdee4f20 100644 --- a/glue/core/component_link.py +++ b/glue/core/component_link.py @@ -18,6 +18,11 @@ def identity(x): return x + +def null(*args): + return None + + OPSYM = {operator.add: '+', operator.sub: '-', operator.truediv: '/', operator.mul: '*', operator.pow: '**'} @@ -382,7 +387,6 @@ def __init__(self, left, right, op): right) to = ComponentID("") - null = lambda *args: None super(BinaryComponentLink, self).__init__(from_, to, null) def replace_ids(self, old, new): From 514a6d26301221733bc96f5f999f3ac86f5130ef Mon Sep 17 00:00:00 2001 From: Thomas Robitaille Date: Fri, 23 Feb 2018 18:25:39 +0000 Subject: [PATCH 09/19] Improve reading in of DataCollection to not add internal data links to the LinkManager --- glue/core/state.py | 48 +++++++++++++++++++++- glue/core/tests/test_data_collection.py | 24 +++++++++++ glue/dialogs/link_editor/qt/link_editor.py | 2 +- 3 files changed, 71 insertions(+), 3 deletions(-) diff --git a/glue/core/state.py b/glue/core/state.py index 21da661ce..912f41aa5 100644 --- a/glue/core/state.py +++ b/glue/core/state.py @@ -637,9 +637,50 @@ def _save_data_collection_3(dc, context): @loader(DataCollection) def _load_data_collection(rec, context): - dc = DataCollection(list(map(context.object, rec['data']))) + + datasets = list(map(context.object, rec['data'])) + links = [context.object(link) for link in rec['links']] - dc.set_links(links) + + # Filter out CoordinateComponentLinks that may have been saved in the past + # as these are now re-generated on-the-fly. + links = [link for link in links if not isinstance(link, CoordinateComponentLink)] + + # Go through and split links into links internal to datasets and ones + # between datasets as this dictates whether they should be set on the + # data collection or on the data objects. + external, internal = [], [] + for link in links: + parent_to = link.get_to_id().parent + for cid in link.get_from_ids(): + if cid.parent is not parent_to: + external.append(link) + break + else: + internal.append(link) + + # Remove components in datasets that have external links + for data in datasets: + remove = [] + for cid in data.derived_components: + comp = data.get_component(cid) + + # Neihter in external nor in links overall + if comp.link not in internal: + remove.append(cid) + + if isinstance(comp.link, CoordinateComponentLink): + remove.append(cid) + + if len(comp.link.get_from_ids()) == 1 and comp.link.get_from_ids()[0].parent is comp.link.get_to_id().parent and comp.link.get_from_ids()[0].label == comp.link.get_to_id().label: + remove.append(cid) + + for cid in remove: + data.remove_component(cid) + + dc = DataCollection(datasets) + + dc.set_links(external) coerce_subset_groups(dc) return dc @@ -715,6 +756,9 @@ def _load_data(rec, context): result._world_component_ids = coord[:len(coord) // 2] result._pixel_component_ids = coord[len(coord) // 2:] + # We can now re-generate the coordinate links + result._set_up_coordinate_component_links(result.ndim) + for s in rec['subsets']: result.add_subset(context.object(s)) diff --git a/glue/core/tests/test_data_collection.py b/glue/core/tests/test_data_collection.py index d1a45387b..e6b7cda13 100644 --- a/glue/core/tests/test_data_collection.py +++ b/glue/core/tests/test_data_collection.py @@ -17,6 +17,7 @@ ComponentsChangedMessage, ExternallyDerivableComponentsChangedMessage) from ..exceptions import IncompatibleAttribute +from .test_state import clone class HubLog(HubListener): @@ -404,3 +405,26 @@ def test_remove_component_message(self): msg = self.log.messages[-1] assert isinstance(msg, ComponentsChangedMessage) + + def test_links_preserved_session(self): + + # This tests that the separation of internal vs external links is + # preserved in session files. + + d1 = Data(a=[1, 2, 3]) + d2 = Data(b=[2, 3, 4]) + + dc = DataCollection([d1, d2]) + dc.add_link(ComponentLink([d2.id['b']], d1.id['a'])) + + d1['x'] = d1.id['a'] + 1 + + assert len(d1.coordinate_links) == 2 + assert len(d1.derived_links) == 1 + assert len(dc._link_manager._external_links) == 1 + + dc2 = clone(dc) + + assert len(dc2[0].coordinate_links) == 2 + assert len(dc2[0].derived_links) == 1 + assert len(dc2._link_manager._external_links) == 1 diff --git a/glue/dialogs/link_editor/qt/link_editor.py b/glue/dialogs/link_editor/qt/link_editor.py index 0405feed4..1696ca07e 100644 --- a/glue/dialogs/link_editor/qt/link_editor.py +++ b/glue/dialogs/link_editor/qt/link_editor.py @@ -106,7 +106,7 @@ def _add_new_link(self): def links(self): current = self._ui.current_links - return current.data.values() + return list(current.data.values()) def _remove_link(self): current = self._ui.current_links From d6192ef2f5ec8c7206c5afd119b10997267b5310 Mon Sep 17 00:00:00 2001 From: Thomas Robitaille Date: Fri, 23 Feb 2018 18:38:44 +0000 Subject: [PATCH 10/19] Only save external links to session file in DataCollection --- glue/core/data_collection.py | 7 +++++ glue/core/link_manager.py | 4 +++ glue/core/state.py | 59 +++++++++++++++++++++++++----------- 3 files changed, 52 insertions(+), 18 deletions(-) diff --git a/glue/core/data_collection.py b/glue/core/data_collection.py index 51d108088..f41475367 100644 --- a/glue/core/data_collection.py +++ b/glue/core/data_collection.py @@ -134,6 +134,13 @@ def links(self): """ return tuple(self._link_manager.links) + @property + def external_links(self): + """ + Tuple of :class:`~glue.core.component_link.ComponentLink` objects. + """ + return tuple(self._link_manager.external_links) + def add_link(self, links): """Add one or more links to the data collection. diff --git a/glue/core/link_manager.py b/glue/core/link_manager.py index a8d5c5f6e..3c2446ed9 100644 --- a/glue/core/link_manager.py +++ b/glue/core/link_manager.py @@ -256,6 +256,10 @@ def _inverse_links(self): def links(self): return list(self._links) + @property + def external_links(self): + return list(self._external_links) + def clear(self): self._external_links.clear() diff --git a/glue/core/state.py b/glue/core/state.py index 912f41aa5..b7e37983b 100644 --- a/glue/core/state.py +++ b/glue/core/state.py @@ -635,6 +635,19 @@ def _save_data_collection_3(dc, context): return result +@saver(DataCollection, version=4) +def _save_data_collection_4(dc, context): + cids = [c for data in dc for c in data.component_ids()] + components = [data.get_component(c) + for data in dc for c in data.component_ids()] + return dict(data=list(map(context.id, dc)), + links=list(map(context.id, dc.external_links)), + cids=list(map(context.id, cids)), + components=list(map(context.id, components)), + groups=list(map(context.id, dc.subset_groups)), + subset_group_count=dc._sg_count) + + @loader(DataCollection) def _load_data_collection(rec, context): @@ -642,22 +655,25 @@ def _load_data_collection(rec, context): links = [context.object(link) for link in rec['links']] - # Filter out CoordinateComponentLinks that may have been saved in the past - # as these are now re-generated on-the-fly. - links = [link for link in links if not isinstance(link, CoordinateComponentLink)] - - # Go through and split links into links internal to datasets and ones - # between datasets as this dictates whether they should be set on the - # data collection or on the data objects. - external, internal = [], [] - for link in links: - parent_to = link.get_to_id().parent - for cid in link.get_from_ids(): - if cid.parent is not parent_to: - external.append(link) - break - else: - internal.append(link) + if rec.get('_protocol', 0) > 3: + external = links + else: + # Filter out CoordinateComponentLinks that may have been saved in the past + # as these are now re-generated on-the-fly. + links = [link for link in links if not isinstance(link, CoordinateComponentLink)] + + # Go through and split links into links internal to datasets and ones + # between datasets as this dictates whether they should be set on the + # data collection or on the data objects. + external, internal = [], [] + for link in links: + parent_to = link.get_to_id().parent + for cid in link.get_from_ids(): + if cid.parent is not parent_to: + external.append(link) + break + else: + internal.append(link) # Remove components in datasets that have external links for data in datasets: @@ -666,8 +682,9 @@ def _load_data_collection(rec, context): comp = data.get_component(cid) # Neihter in external nor in links overall - if comp.link not in internal: - remove.append(cid) + if rec.get('_protocol', 0) <= 3: + if comp.link not in internal: + remove.append(cid) if isinstance(comp.link, CoordinateComponentLink): remove.append(cid) @@ -693,6 +710,7 @@ def _load_data_collection_2(rec, context): grp.register_to_hub(result.hub) return result + @loader(DataCollection, version=3) def _load_data_collection_3(rec, context): result = _load_data_collection_2(rec, context) @@ -700,6 +718,11 @@ def _load_data_collection_3(rec, context): return result +@loader(DataCollection, version=4) +def _load_data_collection_4(rec, context): + return _load_data_collection_3(rec, context) + + @saver(Data) def _save_data(data, context): From 68622b8031f06253d41883e30619892f3955ce24 Mon Sep 17 00:00:00 2001 From: Thomas Robitaille Date: Fri, 23 Feb 2018 19:12:44 +0000 Subject: [PATCH 11/19] Implement new state loader for DataCollection --- glue/core/state.py | 49 +++++++++++++++++++++++++++------------------- 1 file changed, 29 insertions(+), 20 deletions(-) diff --git a/glue/core/state.py b/glue/core/state.py index b7e37983b..b98b14265 100644 --- a/glue/core/state.py +++ b/glue/core/state.py @@ -655,25 +655,22 @@ def _load_data_collection(rec, context): links = [context.object(link) for link in rec['links']] - if rec.get('_protocol', 0) > 3: - external = links - else: - # Filter out CoordinateComponentLinks that may have been saved in the past - # as these are now re-generated on-the-fly. - links = [link for link in links if not isinstance(link, CoordinateComponentLink)] - - # Go through and split links into links internal to datasets and ones - # between datasets as this dictates whether they should be set on the - # data collection or on the data objects. - external, internal = [], [] - for link in links: - parent_to = link.get_to_id().parent - for cid in link.get_from_ids(): - if cid.parent is not parent_to: - external.append(link) - break - else: - internal.append(link) + # Filter out CoordinateComponentLinks that may have been saved in the past + # as these are now re-generated on-the-fly. + links = [link for link in links if not isinstance(link, CoordinateComponentLink)] + + # Go through and split links into links internal to datasets and ones + # between datasets as this dictates whether they should be set on the + # data collection or on the data objects. + external, internal = [], [] + for link in links: + parent_to = link.get_to_id().parent + for cid in link.get_from_ids(): + if cid.parent is not parent_to: + external.append(link) + break + else: + internal.append(link) # Remove components in datasets that have external links for data in datasets: @@ -720,7 +717,19 @@ def _load_data_collection_3(rec, context): @loader(DataCollection, version=4) def _load_data_collection_4(rec, context): - return _load_data_collection_3(rec, context) + + dc = DataCollection(list(map(context.object, rec['data']))) + links = [context.object(link) for link in rec['links']] + dc.set_links(links) + coerce_subset_groups(dc) + + dc._subset_groups = list(map(context.object, rec['groups'])) + for grp in dc.subset_groups: + grp.register_to_hub(dc.hub) + + dc._sg_count = rec['subset_group_count'] + + return dc @saver(Data) From 5111a2fceba2b69f5bb4859f5775670b26165675 Mon Sep 17 00:00:00 2001 From: Thomas Robitaille Date: Fri, 23 Feb 2018 19:16:00 +0000 Subject: [PATCH 12/19] Added a regression test of serializing a BinaryComponentLink --- glue/core/tests/test_state.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/glue/core/tests/test_state.py b/glue/core/tests/test_state.py index e4bfa5163..6c016f492 100644 --- a/glue/core/tests/test_state.py +++ b/glue/core/tests/test_state.py @@ -235,6 +235,14 @@ def test_matplotlib_cmap(): assert clone(cm.gist_heat) is cm.gist_heat +def test_binary_component_link(): + d1 = core.Data(x=[1, 2, 3]) + d1['y'] = d1.id['x'] + 1 + d2 = clone(d1) + assert_equal(d1['x'], [1, 2, 3]) + assert_equal(d2['y'], [2, 3, 4]) + + class Spam(object): pass From d335ac094956978cdd6a186ce864af997facb40f Mon Sep 17 00:00:00 2001 From: Thomas Robitaille Date: Fri, 23 Feb 2018 22:30:58 +0000 Subject: [PATCH 13/19] Fix serialization of binary component link --- glue/core/component_link.py | 23 ++++++++++++++++++----- glue/core/state.py | 11 +++++++++++ glue/core/state_path_patches.txt | 1 + glue/core/tests/test_state.py | 2 +- 4 files changed, 31 insertions(+), 6 deletions(-) diff --git a/glue/core/component_link.py b/glue/core/component_link.py index 3cdee4f20..15b222f2f 100644 --- a/glue/core/component_link.py +++ b/glue/core/component_link.py @@ -401,13 +401,26 @@ def replace_ids(self, old, new): self._right.replace_ids(old, new) def compute(self, data, view=None): - l = self._left - r = self._right + left = self._left + right = self._right if not isinstance(self._left, numbers.Number): - l = data[self._left, view] + left = data[self._left, view] if not isinstance(self._right, numbers.Number): - r = data[self._right, view] - return self._op(l, r) + right = data[self._right, view] + return self._op(left, right) + + def __gluestate__(self, context): + left = context.id(self._left) + right = context.id(self._right) + operator = context.do(self._op) + return dict(left=left, right=right, operator=operator) + + @classmethod + def __setgluestate__(cls, rec, context): + left = context.object(rec['left']) + right = context.object(rec['right']) + operator = context.object(rec['operator']) + return cls(left, right, operator) def __str__(self): sym = OPSYM.get(self._op, self._op.__name__) diff --git a/glue/core/state.py b/glue/core/state.py index b98b14265..604c7c28f 100644 --- a/glue/core/state.py +++ b/glue/core/state.py @@ -995,6 +995,17 @@ def _load_coordinate_component_link(rec, context): return CoordinateComponentLink(frm, to, coords, index, pix2world) +@saver(types.BuiltinFunctionType) +def _save_builtin_function(function, context): + ref = "%s.%s" % (function.__module__, function.__name__) + return {'function': ref} + + +@loader(types.BuiltinFunctionType) +def _load_builtin_function(rec, context): + return lookup_class_with_patches(rec['function']) + + @saver(types.FunctionType) def _save_function(function, context): ref = "%s.%s" % (function.__module__, function.__name__) diff --git a/glue/core/state_path_patches.txt b/glue/core/state_path_patches.txt index 66b132a09..44108609a 100644 --- a/glue/core/state_path_patches.txt +++ b/glue/core/state_path_patches.txt @@ -24,3 +24,4 @@ glue.viewers.table.qt.viewer_widget.TableWidget -> glue.viewers.table.qt.TableVi glue_vispy_viewers.common.toolbar.PatchedElementSubsetState -> glue.core.subset.ElementSubsetState glue.plugins.dendro_viewer.qt.viewer_widget.DendroWidget -> glue.plugins.dendro_viewer.qt.DendrogramViewer glue.plugins.dendro_viewer.layer_artist.DendroLayerArtist -> glue.plugins.dendro_viewer.layer_artist.DendrogramLayerArtist +builtins.builtin_function_or_method -> types.BuiltinFunctionType diff --git a/glue/core/tests/test_state.py b/glue/core/tests/test_state.py index 6c016f492..9d209a779 100644 --- a/glue/core/tests/test_state.py +++ b/glue/core/tests/test_state.py @@ -238,8 +238,8 @@ def test_matplotlib_cmap(): def test_binary_component_link(): d1 = core.Data(x=[1, 2, 3]) d1['y'] = d1.id['x'] + 1 + assert_equal(d1['y'], [2, 3, 4]) d2 = clone(d1) - assert_equal(d1['x'], [1, 2, 3]) assert_equal(d2['y'], [2, 3, 4]) From 76393b3142bbb8e11d19d9874608de973572bda8 Mon Sep 17 00:00:00 2001 From: Thomas Robitaille Date: Sat, 24 Feb 2018 12:47:18 +0000 Subject: [PATCH 14/19] Fix tests on Python 2.7 --- glue/core/state_path_patches.txt | 1 + glue/core/tests/test_data_collection.py | 15 ++++++++++++--- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/glue/core/state_path_patches.txt b/glue/core/state_path_patches.txt index 44108609a..70c57e5e9 100644 --- a/glue/core/state_path_patches.txt +++ b/glue/core/state_path_patches.txt @@ -25,3 +25,4 @@ glue_vispy_viewers.common.toolbar.PatchedElementSubsetState -> glue.core.subset. glue.plugins.dendro_viewer.qt.viewer_widget.DendroWidget -> glue.plugins.dendro_viewer.qt.DendrogramViewer glue.plugins.dendro_viewer.layer_artist.DendroLayerArtist -> glue.plugins.dendro_viewer.layer_artist.DendrogramLayerArtist builtins.builtin_function_or_method -> types.BuiltinFunctionType +__builtin__.builtin_function_or_method -> types.BuiltinFunctionType diff --git a/glue/core/tests/test_data_collection.py b/glue/core/tests/test_data_collection.py index e6b7cda13..bd7f9faf7 100644 --- a/glue/core/tests/test_data_collection.py +++ b/glue/core/tests/test_data_collection.py @@ -13,12 +13,13 @@ from ..data_collection import DataCollection from ..hub import HubListener from ..message import (Message, DataCollectionAddMessage, DataRemoveComponentMessage, - DataCollectionDeleteMessage, + DataCollectionDeleteMessage, DataAddComponentMessage, ComponentsChangedMessage, ExternallyDerivableComponentsChangedMessage) from ..exceptions import IncompatibleAttribute from .test_state import clone + class HubLog(HubListener): def __init__(self): @@ -30,6 +31,9 @@ def register_to_hub(self, hub): def notify(self, message): self.messages.append(message) + def clear(self): + self.messages[:] = [] + class TestDataCollection(object): @@ -159,10 +163,15 @@ def test_catch_data_add_component_message(self): self.dc.append(d) d.add_component(Component(np.array([1, 2, 3])), id1) assert link not in self.dc._link_manager + self.log.clear() d.add_component(dc, id2) - msg = self.log.messages[-1] - assert isinstance(msg, ComponentsChangedMessage) + msgs = sorted(self.log.messages, key=lambda x: str(type(x))) + + assert isinstance(msgs[0], ComponentsChangedMessage) + assert isinstance(msgs[1], DataAddComponentMessage) + assert isinstance(msgs[2], ExternallyDerivableComponentsChangedMessage) + assert link in self.dc._link_manager def test_links_auto_added(self): From a1ccd41d8e4d7a57724a5b1fe75677ede17b1253 Mon Sep 17 00:00:00 2001 From: Thomas Robitaille Date: Sat, 24 Feb 2018 16:09:41 +0000 Subject: [PATCH 15/19] Don't raise unnecessary ExternallyDerivableComponentsChangedMessages --- glue/core/data.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/glue/core/data.py b/glue/core/data.py index 3a5fe7aff..2ce571fce 100644 --- a/glue/core/data.py +++ b/glue/core/data.py @@ -425,6 +425,7 @@ def add_component(self, component, label, hidden=False): component_id = ComponentID(label, hidden=hidden, parent=self) if len(self._components) == 0: + # TODO: make sure the following doesn't raise a componentsraised message self._create_pixel_and_world_components(ndim=component.ndim) # In some cases, such as when loading a session, we actually disable the @@ -457,6 +458,8 @@ def _set_externally_derivable_components(self, derivable_components): values are DerivedComponent instances which can be used to get the data. """ + if len(self._externally_derivable_components) == 0 and len(derivable_components) == 0: + return self._externally_derivable_components = derivable_components if self.hub: msg = ExternallyDerivableComponentsChangedMessage(self) From 4f987a76bb42139a8f6006b7cee7a65f35377b26 Mon Sep 17 00:00:00 2001 From: Thomas Robitaille Date: Sat, 24 Feb 2018 16:09:46 +0000 Subject: [PATCH 16/19] Fix another test --- glue/core/tests/test_data_collection.py | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/glue/core/tests/test_data_collection.py b/glue/core/tests/test_data_collection.py index bd7f9faf7..5d57f2fa9 100644 --- a/glue/core/tests/test_data_collection.py +++ b/glue/core/tests/test_data_collection.py @@ -401,19 +401,18 @@ def test_remove_component_message(self): remove_id = data.id['y'] + self.log.clear() + data.remove_component(remove_id) - print(self.log.messages) + msgs = sorted(self.log.messages, key=lambda x: str(type(x))) - msg = self.log.messages[-3] - assert isinstance(msg, DataRemoveComponentMessage) - assert msg.component_id is remove_id + print([type(msg) for msg in msgs]) - msg = self.log.messages[-2] - assert isinstance(msg, ExternallyDerivableComponentsChangedMessage) + assert isinstance(msgs[0], ComponentsChangedMessage) - msg = self.log.messages[-1] - assert isinstance(msg, ComponentsChangedMessage) + assert isinstance(msgs[1], DataRemoveComponentMessage) + assert msgs[1].component_id is remove_id def test_links_preserved_session(self): From d26d1cb071eb44fb6bfcd1a1a37c9879ab45796f Mon Sep 17 00:00:00 2001 From: Thomas Robitaille Date: Sun, 25 Feb 2018 10:36:13 +0000 Subject: [PATCH 17/19] Fix documentation warnings --- glue/core/data.py | 27 ++++-- glue/core/fitters.py | 2 +- glue/core/hub.py | 2 - glue/core/simpleforms.py | 86 +++++++++--------- glue/core/util.py | 192 +++++++++++++++++++++++---------------- 5 files changed, 176 insertions(+), 133 deletions(-) diff --git a/glue/core/data.py b/glue/core/data.py index 2ce571fce..5527bbb19 100644 --- a/glue/core/data.py +++ b/glue/core/data.py @@ -468,16 +468,23 @@ def _set_externally_derivable_components(self, derivable_components): @contract(link=ComponentLink, label='cid_like|None', returns=DerivedComponent) - def add_component_link(self, link, label=None, hidden=False): - """ Shortcut method for generating a new :class:`~glue.core.component.DerivedComponent` - from a ComponentLink object, and adding it to a data set. - - :param link: :class:`~glue.core.component_link.ComponentLink` - :param label: The ComponentID or label to attach to. - :type label: :class:`~glue.core.component_id.ComponentID` or str + def add_component_link(self, link, label=None): + """ + Shortcut method for generating a new + :class:`~glue.core.component.DerivedComponent` from a ComponentLink + object, and adding it to a data set. - :returns: - The :class:`~glue.core.component.DerivedComponent` that was added + Parameters + ---------- + link : :class:`~glue.core.component_link.ComponentLink` + The link to use to generate a new component + label : :class:`~glue.core.component_id.ComponentID` or str + The ComponentID or label to attach to. + + Returns + ------- + component : :class:`~glue.core.component.DerivedComponent` + The component that was added """ if label is not None: if not isinstance(label, ComponentID): @@ -554,7 +561,7 @@ def externally_derivable_components(self): @property def visible_components(self): - """ :class:`ComponentIDs ` for all non-hidden components. + """All :class:`ComponentIDs ` in the Data that aren't coordinate. :rtype: list """ diff --git a/glue/core/fitters.py b/glue/core/fitters.py index 3a21b7679..e06b05494 100644 --- a/glue/core/fitters.py +++ b/glue/core/fitters.py @@ -149,7 +149,7 @@ def fit(self, x, y, dy, constraints, **options): :type y: :class:`numpy.ndarray` :param dy: 1 sigma uncertainties on each datum (optional) :type dy: :class:`numpy.ndarray` - :param constraints: The current value of :attr:`constraints` + :param constraints: The current value of the ``constraints`` property :param options: kwargs for model hyperparameters. :returns: An object representing the fit result. diff --git a/glue/core/hub.py b/glue/core/hub.py index 2756f0f00..e107983b3 100644 --- a/glue/core/hub.py +++ b/glue/core/hub.py @@ -83,14 +83,12 @@ def subscribe(self, subscriber, message_class, An optional function of the form handler(message) that will receive the message on behalf of the subscriber. If not provided, this defaults to the HubListener's notify method - :type handler: Callable :param filter: An optional function of the form filter(message). Messages are only passed to the subscriber if filter(message) == True. The default is to always pass messages. - :type filter: Callable Raises: diff --git a/glue/core/simpleforms.py b/glue/core/simpleforms.py index b77a71490..77e691efd 100644 --- a/glue/core/simpleforms.py +++ b/glue/core/simpleforms.py @@ -11,20 +11,20 @@ class Option(object): - """ Base class for other options. This shouldn't be used directly + + Parameters + ---------- + default : object + The default value for this option. + label : str + A short label for this option, to use in the GUI """ def __init__(self, default, label): - """ - :param default: The default value for this option. - :type default: object - :param label: A short label for this option, to use in the GUI - :type label: str - """ self.label = label """A UI label for the setting""" self.default = default @@ -46,22 +46,23 @@ def _validate(self, value): class IntOption(Option): + """ + An integer-valued option. + + Parameters + ---------- + min : int, optional + The minimum valid value + max : int, optional + The maximum valid value + default : int, optional + The default value + label : str, optional + A short label for this option + """ def __init__(self, min=0, max=10, default=1, label="Integer"): - """ - An integer-valued option - - :param min: The minimum valid value - :type min: integer - :param max: The maximum valid value - :type max: integer - :param default: The default value - :type default: integer - :param label: A short label for this option - :type label: str - """ super(IntOption, self).__init__(default, label) - self.min = min self.max = max @@ -84,20 +85,22 @@ def _validate(self, value): class FloatOption(Option): + """ + A floating-point option. + + Parameters + ---------- + min : float, optional + The minimum valid value + max : float, optional + The maximum valid value + default : float, optional + The default value + label : str, optional + A short label for this option + """ def __init__(self, min=0, max=10, default=1, label="Float"): - """ - An floating-point option - - :param min: The minimum valid value - :type min: float - :param max: The maximum valid value - :type max: float - :param default: The default value - :type default: float - :param label: A short label for this option - :type label: str - """ super(FloatOption, self).__init__(default, label) self.min = min self.max = max @@ -112,17 +115,18 @@ def _validate(self, value): class BoolOption(Option): + """ + A boolean-valued option. + + Parameters + ---------- + label : str, optional + A short label for this option + default : bool, optional + The default `True`/`False` value + """ def __init__(self, label="Bool", default=False): - """ - A boolean-valued option - - :param default: The default True/False value - :type default: bool - - :param label: A short label for this option - :type label: str - """ super(BoolOption, self).__init__(default, label) def _validate(self, value): diff --git a/glue/core/util.py b/glue/core/util.py index 07c54f69e..9658ad2c6 100644 --- a/glue/core/util.py +++ b/glue/core/util.py @@ -33,18 +33,21 @@ def relim(lo, hi, log=False): def split_component_view(arg): - """Split the input to data or subset.__getitem__ into its pieces. - - :param arg: The input passed to data or subset.__getitem__. - Assumed to be either a scalar or tuple - - :rtype: tuple - - The first item is the Component selection (a ComponentID or - string) - - The second item is a view (tuple of slices, slice scalar, or view - object) + """ + Split the input to data or subset.__getitem__ into its pieces. + + Parameters + ---------- + arg + The input passed to ``data`` or ``subset.__getitem__``. Assumed to be + either a scalar or tuple + + Returns + ------- + selection + The Component selection (a ComponentID or string) + view + Tuple of slices, slice scalar, or view """ if isinstance(arg, tuple): if len(arg) == 1: @@ -58,14 +61,18 @@ def split_component_view(arg): def join_component_view(component, view): - """Pack a componentID and optional view into single tuple + """ + Pack a ComponentID and optional view into single tuple. - Returns an object compatible with data.__getitem__ and related - methods. Handles edge cases of when view is None, a scalar, a - tuple, etc. + Returns an object compatible with ``data.__getitem__`` and related methods. + Handles edge cases of when view is None, a scalar, a tuple, etc. - :param component: ComponentID - :param view: view into data, or None + Parameters + ---------- + component : `~glue.core.component_id.ComponentID` + The ComponentID to pack + view + The view into the data, or `None` """ if view is None: @@ -81,39 +88,40 @@ def join_component_view(component, view): def facet_subsets(data_collection, cid, lo=None, hi=None, steps=5, prefix='', log=False): - """Create a series of subsets that partition the values of - a particular attribute into several bins - - This creates `steps` new subet groups, adds them to the data collection, - and returns the list of newly created subset groups. - - :param data: DataCollection object to use - :type data: :class:`~glue.core.data_collection.DataCollection` - - :param cid: ComponentID to facet on - :type data: :class:`~glue.core.component_id.ComponentID` - - :param lo: The lower bound for the faceting. Defaults to minimum value - in data - :type lo: float - - :param hi: The upper bound for the faceting. Defaults to maximum - value in data - :type hi: float - - :param steps: The number of subsets to create. Defaults to 5 - :type steps: int - - :param prefix: If present, the new subset labels will begin with `prefix` - :type prefix: str - - :param log: If True, space divisions logarithmically. Default=False - :type log: bool - - :returns: List of :class:`~glue.core.subset_group.SubsetGroup` instances - added to `data` - - Example:: + """ + Create a series of subsets that partition the values of a particular + attribute into several bins + + This creates `steps` new subet groups, adds them to the data collection, and + returns the list of newly created subset groups. + + Parameters + ---------- + data : :class:`~glue.core.data_collection.DataCollection` + The DataCollection object to use + cid : :class:`~glue.core.component_id.ComponentID` + The ComponentID to facet on + lo : float, optional + The lower bound for the faceting. Defaults to minimum value in data + hi : float, optional + The upper bound for the faceting. Defaults to maximum value in data + steps : int, optional + The number of subsets to create. Defaults to 5 + prefix : str, optional + If present, the new subset labels will begin with `prefix` + log : bool, optional + If `True`, space divisions logarithmically. Default is `False` + + Returns + ------- + subset_groups : iterable + List of :class:`~glue.core.subset_group.SubsetGroup` instances added to + `data` + + Examples + -------- + + :: facet_subset(data, data.id['mass'], lo=0, hi=10, steps=2) @@ -132,7 +140,6 @@ def facet_subsets(data_collection, cid, lo=None, hi=None, steps=5, Note that the last range is inclusive on both sides. For example, if ``lo`` is 0 and ``hi`` is 5, and ``steps`` is 5, then the intervals for the subsets are [0,1), [1,2), [2,3), [3,4), and [4,5]. - """ from glue.core.exceptions import IncompatibleAttribute if lo is None or hi is None: @@ -188,16 +195,23 @@ def facet_subsets(data_collection, cid, lo=None, hi=None, steps=5, def colorize_subsets(subsets, cmap, lo=0, hi=1): - """Re-color a list of subsets according to a colormap - - :param subsets: List of subsets - :param cmap: Matplotlib colormap instance - :param lo: Start location in colormap. 0-1. Defaults to 0 - :param hi: End location in colormap. 0-1. Defaults to 1 + """ + Re-color a list of subsets according to a colormap. The colormap will be sampled at `len(subsets)` even intervals between `lo` and `hi`. The color at the `ith` interval will be applied to `subsets[i]` + + Parameters + ---------- + subsets : list + List of subsets + cmap : `~matplotlib.colors.Colormap` + Matplotlib colormap instance + lo : float, optional + Start location in colormap. 0-1. Defaults to 0 + hi : float, optional + End location in colormap. 0-1. Defaults to 1 """ from matplotlib import cm @@ -217,13 +231,18 @@ def colorize_subsets(subsets, cmap, lo=0, hi=1): def disambiguate(label, taken): - """If necessary, add a suffix to label to avoid name conflicts + """ + If necessary, add a suffix to label to avoid name conflicts - :param label: desired label - :param taken: set of taken names + Returns label if it is not in the taken set. Otherwise, returns label_NN + where NN is the lowest integer such that label_NN not in taken. - Returns label if it is not in the taken set. Otherwise, returns - label_NN where NN is the lowest integer such that label_NN not in taken. + Parameters + ---------- + label : str + Desired label + taken : iterable + The set of already taken names """ if label not in taken: return label @@ -239,12 +258,18 @@ def row_lookup(data, categories): """ Lookup which row in categories each data item is equal to - :param data: array-like - :param categories: array-like of unique values - - :returns: Float array. - If result[i] is finite, then data[i] = categoreis[result[i]] - Otherwise, data[i] is not in the categories list + Parameters + ---------- + data + An array-like object + categories + Array-like of unique values + + Returns + ------- + array + If result[i] is finite, then data[i] = categoreis[result[i]] + Otherwise, data[i] is not in the categories list """ # np.searchsorted doesn't work on mixed types in Python3 @@ -262,8 +287,7 @@ def row_lookup(data, categories): def small_view(data, attribute): """ - Extract a downsampled view from a dataset, for quick - statistical summaries + Extract a downsampled view from a dataset, for quick statistical summaries """ shp = data.shape view = tuple([slice(None, None, np.intp(max(s / 50, 1))) for s in shp]) @@ -290,8 +314,12 @@ def visible_limits(artists, axis): Returns a tuple of min, max for the requested axis, or None if no data present - :param artists: An iterable collection of artists - :param axis: Which axis to compute. 0=xaxis, 1=yaxis + Parameters + ---------- + artists : iterable + An iterable collection of artists + axis : int + Which axis to compute. 0=xaxis, 1=yaxis """ data = [] for art in artists: @@ -339,12 +367,18 @@ def update_ticks(axes, coord, components, is_log): Changes the axes to have the proper tick formatting based on the type of component. - :param axes: A matplotlib axis object to alter - :param coord: 'x' or 'y' - :param components: A list() of components that are plotted along this axis - :param is_log: Boolean for log-scale. - :kwarg max_categories: The maximum number of categories to display. - :return: None or #categories if components is Categorical + Returns `None` or the number of categories if components is Categorical. + + Parameters + ---------- + axes : `~matplotlib.axes.Axes` + A matplotlib axis object to alter + coord : { 'x' | 'y' } + The coordinate axis on which to update the ticks + components : iterable + A list of components that are plotted along this axis + if_log : boolean + Whether the axis has a log-scale """ if coord == 'x': From 9e24d60a49acbd3a05243f9c585c50eeb83d315c Mon Sep 17 00:00:00 2001 From: Thomas Robitaille Date: Sun, 25 Feb 2018 18:04:44 +0000 Subject: [PATCH 18/19] Remove the concept of a 'hidden' component since this is no longer needed --- glue/core/component.py | 9 --- glue/core/component_id.py | 23 +++---- glue/core/component_link.py | 2 - glue/core/data.py | 62 ++++++++--------- glue/core/data_combo_helper.py | 3 +- glue/core/link_manager.py | 3 +- glue/core/state.py | 15 ++-- glue/core/tests/test_data.py | 69 ++++++++----------- glue/core/tests/test_data_collection.py | 5 +- glue/core/tests/test_link_manager.py | 4 +- .../component_manager/qt/component_manager.py | 13 ++-- glue/viewers/custom/qt/custom_viewer.py | 3 +- 12 files changed, 86 insertions(+), 125 deletions(-) diff --git a/glue/core/component.py b/glue/core/component.py index 53b4f52c0..35bd3144c 100644 --- a/glue/core/component.py +++ b/glue/core/component.py @@ -67,11 +67,6 @@ def units(self, value): else: self._units = str(value) - @property - def hidden(self): - """Whether the Component is hidden by default""" - return False - @property def data(self): """ The underlying :class:`numpy.ndarray` """ @@ -250,10 +245,6 @@ def set_parent(self, data): """ Reassign the Data object that this DerivedComponent operates on """ self._data = data - @property - def hidden(self): - return self._link.hidden - @property def data(self): """ Return the numerical data as a numpy array """ diff --git a/glue/core/component_id.py b/glue/core/component_id.py index e05c0b0fe..bdaca9f40 100644 --- a/glue/core/component_id.py +++ b/glue/core/component_id.py @@ -48,13 +48,15 @@ class ComponentID(object): component_id = data.id[name] data[component_id] -> numpy array + + Parameters + ---------- + label : str + Name for the component ID """ - def __init__(self, label, hidden=False, parent=None): - """:param label: Name for the ID - :type label: str""" + def __init__(self, label, parent=None): self._label = str(label) - self._hidden = hidden self.parent = parent # We assign a UUID which can then be used for example in equations # for derived components - the idea is that this doesn't change over @@ -84,15 +86,6 @@ def label(self, value): msg = DataRenameComponentMessage(self.parent, self) self.parent.hub.broadcast(msg) - @property - def hidden(self): - """Whether to hide the component by default""" - return self._hidden - - @hidden.setter - def hidden(self, value): - self._hidden = value - def __str__(self): return str(self._label) @@ -176,6 +169,6 @@ class PixelComponentID(ComponentID): dimensions. """ - def __init__(self, axis, label, hidden=False, parent=None): + def __init__(self, axis, label, parent=None): self.axis = axis - super(PixelComponentID, self).__init__(label, hidden=hidden, parent=parent) + super(PixelComponentID, self).__init__(label, parent=parent) diff --git a/glue/core/component_link.py b/glue/core/component_link.py index 15b222f2f..430b26a25 100644 --- a/glue/core/component_link.py +++ b/glue/core/component_link.py @@ -98,7 +98,6 @@ def __init__(self, comp_from, comp_to, using=None, inverse=None, inverse_compone self._using = using self._inverse = inverse - self.hidden = False # show in widgets? self.identity = self._using is identity if not isinstance(comp_from, list): @@ -322,7 +321,6 @@ def __init__(self, comp_from, comp_to, coords, index, pixel2world=True): comp_from = [comp_from[i] for i in self.from_needed] super(CoordinateComponentLink, self).__init__( comp_from, comp_to, self.using) - self.hidden = True def using(self, *args): diff --git a/glue/core/data.py b/glue/core/data.py index 5527bbb19..d1a744885 100644 --- a/glue/core/data.py +++ b/glue/core/data.py @@ -376,7 +376,7 @@ def get_component_id(data, name): other._key_joins[self] = (cid_other, cid) @contract(component='component_like', label='cid_like') - def add_component(self, component, label, hidden=False): + def add_component(self, component, label): """ Add a new component to this data set. :param component: object to add. Can be a Component, @@ -402,7 +402,7 @@ def add_component(self, component, label, hidden=False): """ if isinstance(component, ComponentLink): - return self.add_component_link(component, label=label, hidden=hidden) + return self.add_component_link(component, label=label) if not isinstance(component, Component): component = Component.autotyped(component) @@ -422,7 +422,7 @@ def add_component(self, component, label, hidden=False): if component_id.parent is None: component_id.parent = self else: - component_id = ComponentID(label, hidden=hidden, parent=self) + component_id = ComponentID(label, parent=self) if len(self._components) == 0: # TODO: make sure the following doesn't raise a componentsraised message @@ -458,8 +458,10 @@ def _set_externally_derivable_components(self, derivable_components): values are DerivedComponent instances which can be used to get the data. """ + print("_set_externally_derivable_components", len(derivable_components)) if len(self._externally_derivable_components) == 0 and len(derivable_components) == 0: return + print("HERE", self.hub) self._externally_derivable_components = derivable_components if self.hub: msg = ExternallyDerivableComponentsChangedMessage(self) @@ -488,7 +490,7 @@ def add_component_link(self, link, label=None): """ if label is not None: if not isinstance(label, ComponentID): - label = ComponentID(label, parent=self, hidden=hidden) + label = ComponentID(label, parent=self) link.set_to_id(label) if link.get_to_id() is None: @@ -497,11 +499,12 @@ def add_component_link(self, link, label=None): for cid in link.get_from_ids(): if cid not in self.components: - raise ValueError("Can only add internal links with add_component_link - use DataCollection.add_link to add inter-data links") + raise ValueError("Can only add internal links with add_component_link " + "- use DataCollection.add_link to add inter-data links") dc = DerivedComponent(self, link) to_ = link.get_to_id() - self.add_component(dc, label=to_, hidden=hidden) + self.add_component(dc, label=to_) return dc def _create_pixel_and_world_components(self, ndim): @@ -509,7 +512,7 @@ def _create_pixel_and_world_components(self, ndim): for i in range(ndim): comp = CoordinateComponent(self, i) label = pixel_label(i, ndim) - cid = PixelComponentID(i, "Pixel Axis %s" % label, hidden=True, parent=self) + cid = PixelComponentID(i, "Pixel Axis %s" % label, parent=self) self.add_component(comp, cid) self._pixel_component_ids.append(cid) @@ -517,7 +520,7 @@ def _create_pixel_and_world_components(self, ndim): for i in range(ndim): comp = CoordinateComponent(self, i, world=True) label = self.coords.axis_label(i) - cid = self.add_component(comp, label, hidden=True) + cid = self.add_component(comp, label) self._world_component_ids.append(cid) self._set_up_coordinate_component_links(ndim) @@ -566,7 +569,7 @@ def visible_components(self): :rtype: list """ return [cid for cid, comp in self._components.items() - if not cid.hidden and not comp.hidden and cid.parent is self] + if not isinstance(comp, CoordinateComponent) and cid.parent is self] @property def coordinate_components(self): @@ -577,6 +580,11 @@ def coordinate_components(self): return [c for c in self.component_ids() if isinstance(self._components[c], CoordinateComponent)] + @property + def main_components(self): + return [c for c in self.component_ids() if + not isinstance(self._components[c], (DerivedComponent, CoordinateComponent))] + @property def primary_components(self): """The ComponentIDs not associated with a :class:`~glue.core.component.DerivedComponent` @@ -624,7 +632,7 @@ def find_component_id(self, label): one takes precedence. """ - for cid_set in (self.primary_components, self.derived_components, list(self._externally_derivable_components)): + for cid_set in (self.primary_components, self.derived_components, self.coordinate_components, list(self._externally_derivable_components)): result = [] for cid in cid_set: @@ -766,7 +774,7 @@ def broadcast(self, attribute): Send a :class:`~glue.core.message.DataUpdateMessage` to the hub :param attribute: Name of an attribute that has changed (or None) - :type attribute: string + :type attribute: str """ if not self.hub: return @@ -817,8 +825,6 @@ def update_id(self, old, new): pass if changed and self.hub is not None: - # promote hidden status - new._hidden = new.hidden and old.hidden # remove old component and broadcast the change # see #508 for discussion of this @@ -829,24 +835,18 @@ def __str__(self): s = "Data Set: %s\n" % self.label s += "Number of dimensions: %i\n" % self.ndim s += "Shape: %s\n" % ' x '.join([str(x) for x in self.shape]) - for hidden in [False, True]: - if hidden: - s += "Hidden " - else: - s += "Main " - s += "components:\n" - if hidden: - components = [c for c in self.components if c not in self.visible_components] - else: - components = [c for c in self.visible_components] - for i, cid in enumerate(components): - if cid.hidden != hidden: - continue - comp = self.get_component(cid) - if comp.units is None or comp.units == '': - s += " %i) %s\n" % (i, cid) - else: - s += " %i) %s [%s]\n" % (i, cid, comp.units) + categories = [('Main', self.main_components), + ('Derived', self.derived_components), + ('Coordinate', self.coordinate_components)] + for category, components in categories: + if len(components) > 0: + s += category + " components:\n" + for cid in components: + comp = self.get_component(cid) + if comp.units is None or comp.units == '': + s += " - {0}\n".format(cid) + else: + s += " - {0} [{1}]\n".format(cid, comp.units) return s[:-1] def __repr__(self): diff --git a/glue/core/data_combo_helper.py b/glue/core/data_combo_helper.py index 858d3e1c1..4104a02b2 100644 --- a/glue/core/data_combo_helper.py +++ b/glue/core/data_combo_helper.py @@ -311,8 +311,7 @@ def refresh(self, *args): if self.numeric and self.derived: cids = [ChoiceSeparator('Derived components')] for cid in derived_components: - if not cid.hidden: - cids.append(cid) + cids.append(cid) if len(cids) > 1: choices += cids diff --git a/glue/core/link_manager.py b/glue/core/link_manager.py index 3c2446ed9..ecdb4eb0b 100644 --- a/glue/core/link_manager.py +++ b/glue/core/link_manager.py @@ -51,7 +51,8 @@ def accessible_links(cids, links): def discover_links(data, links): - """ Discover all links to components that can be derived + """ + Discover all links to components that can be derived based on the current components known to a dataset, and a set of ComponentLinks. diff --git a/glue/core/state.py b/glue/core/state.py index 604c7c28f..05ab56304 100644 --- a/glue/core/state.py +++ b/glue/core/state.py @@ -772,8 +772,7 @@ def _load_data(rec, context): # and upgrade it to one. This can be removed once we no longer # support pre-v0.8 session files. if not comp.world and not isinstance(cid, PixelComponentID): - cid = PixelComponentID(comp.axis, cid.label, - hidden=cid.hidden, parent=cid.parent) + cid = PixelComponentID(comp.axis, cid.label, parent=cid.parent) comps[icomp] = (cid, comp) result.add_component(comp, cid) @@ -873,17 +872,17 @@ def load_cid_tuple(cids): @saver(ComponentID) def _save_component_id(cid, context): - return dict(label=cid.label, hidden=cid.hidden) + return dict(label=cid.label) @loader(ComponentID) def _load_component_id(rec, context): - return ComponentID(rec['label'], rec['hidden']) + return ComponentID(rec['label']) @saver(PixelComponentID) def _save_pixel_component_id(cid, context): - return dict(axis=cid.axis, label=cid.label, hidden=cid.hidden) + return dict(axis=cid.axis, label=cid.label) @loader(PixelComponentID) @@ -892,7 +891,7 @@ def _load_pixel_component_id(rec, context): axis = rec['axis'] else: # backward-compatibility axis = int(rec['label'].split()[2]) - return PixelComponentID(axis, rec['label'], rec['hidden']) + return PixelComponentID(axis, rec['label']) @saver(Component) @@ -959,8 +958,7 @@ def _save_component_link(link, context): to = list(map(context.id, [link.get_to_id()])) using = context.do(link.get_using()) inverse = context.do(link.get_inverse()) - hidden = link.hidden - return dict(frm=frm, to=to, using=using, inverse=inverse, hidden=hidden) + return dict(frm=frm, to=to, using=using, inverse=inverse) @loader(ComponentLink) @@ -970,7 +968,6 @@ def _load_component_link(rec, context): using = context.object(rec['using']) inverse = context.object(rec['inverse']) result = ComponentLink(frm, to, using, inverse) - result.hidden = rec['hidden'] return result diff --git a/glue/core/tests/test_data.py b/glue/core/tests/test_data.py index 72386e299..0be8e63a0 100644 --- a/glue/core/tests/test_data.py +++ b/glue/core/tests/test_data.py @@ -95,7 +95,7 @@ def test_add_component_incompatible_shape(self): assert exc.value.args[0] == ("add_component() missing 1 required " "positional argument: 'label'") else: - assert exc.value.args[0] == ("add_component() takes at least 3 " + assert exc.value.args[0] == ("add_component() takes exactly 3 " "arguments (2 given)") def test_get_getitem_incompatible_attribute(self): @@ -634,11 +634,11 @@ def test_add_binary_component(): Number of dimensions: 1 Shape: 3 Main components: - 0) x - 1) y -Hidden components: - 0) Pixel Axis 0 [x] - 1) World 0 + - x + - y +Coordinate components: + - Pixel Axis 0 [x] + - World 0 """.strip() @@ -648,6 +648,27 @@ def test_data_str(): assert str(d) == EXPECTED_STR +EXPECTED_STR_WITH_DERIVED = """ +Data Set: mydata +Number of dimensions: 1 +Shape: 3 +Main components: + - x + - y +Derived components: + - z +Coordinate components: + - Pixel Axis 0 [x] + - World 0 +""".strip() + + +def test_data_str_with_derived(): + d = Data(x=[1, 2, 3], y=[2, 3, 4], label='mydata') + d['z'] = d.id['x'] + 1 + assert str(d) == EXPECTED_STR_WITH_DERIVED + + def test_update_values_from_data(): d1 = Data(a=[1, 2, 3], b=[4, 5, 6], label='banana') d2 = Data(b=[1, 2, 3, 4], c=[5, 6, 7, 8], label='apple') @@ -717,42 +738,6 @@ def test_find_component_id_with_cid(): assert d1.find_component_id(d1.id['b']) is d1.id['b'] -def test_linked_component_visible(): - - # Regression test for a bug that caused components to become hidden once - # they were linked with another component. - - from ..link_helpers import LinkSame - from ..data_collection import DataCollection - - d1 = Data(x=[1], y=[2]) - d2 = Data(w=[3], v=[4]) - - assert not d1.id['x'].hidden - assert not d2.id['w'].hidden - - dc = DataCollection([d1, d2]) - dc.add_link(LinkSame(d1.id['x'], d2.id['w'])) - - assert d1.id['x'] is d2.id['x'] - assert d1.id['w'] is d2.id['w'] - - assert not d1.id['x'].hidden - assert not d2.id['w'].hidden - - assert not d1.id['w'].hidden - assert not d2.id['x'].hidden - - assert d1.id['x'].parent is d1 - assert d1.id['y'].parent is d1 - - assert d2.id['w'].parent is d2 - assert d2.id['v'].parent is d2 - - assert d1.visible_components == [d1.id['x'], d1.id['y']] - assert d2.visible_components == [d2.id['v'], d2.id['w']] - - def test_parent_preserved_session(): # Regression test for a bug that caused ComponentID parents to not be diff --git a/glue/core/tests/test_data_collection.py b/glue/core/tests/test_data_collection.py index 5d57f2fa9..f74d2e694 100644 --- a/glue/core/tests/test_data_collection.py +++ b/glue/core/tests/test_data_collection.py @@ -166,13 +166,12 @@ def test_catch_data_add_component_message(self): self.log.clear() d.add_component(dc, id2) + assert link in self.dc._link_manager + msgs = sorted(self.log.messages, key=lambda x: str(type(x))) assert isinstance(msgs[0], ComponentsChangedMessage) assert isinstance(msgs[1], DataAddComponentMessage) - assert isinstance(msgs[2], ExternallyDerivableComponentsChangedMessage) - - assert link in self.dc._link_manager def test_links_auto_added(self): id1 = ComponentID("id1") diff --git a/glue/core/tests/test_link_manager.py b/glue/core/tests/test_link_manager.py index efbe31354..750d85446 100644 --- a/glue/core/tests/test_link_manager.py +++ b/glue/core/tests/test_link_manager.py @@ -86,7 +86,7 @@ def test_correct_discover(self): links = discover_links(self.data, self.links) for i in self.inaccessible: - assert not i in links + assert i not in links for d in self.direct: assert d in links @@ -95,7 +95,7 @@ def test_correct_discover(self): assert d in links for p in self.primary: - assert not p in links + assert p not in links def test_links_point_to_proper_ids(self): """ Dictionary values are ComponentLinks which diff --git a/glue/dialogs/component_manager/qt/component_manager.py b/glue/dialogs/component_manager/qt/component_manager.py index be53cb975..f3edaba1f 100644 --- a/glue/dialogs/component_manager/qt/component_manager.py +++ b/glue/dialogs/component_manager/qt/component_manager.py @@ -88,13 +88,12 @@ def __init__(self, data_collection=None, parent=None): for data in data_collection: - for cid in data.primary_components: - if not cid.hidden: - comp_state = {} - comp_state['cid'] = cid - comp_state['label'] = cid.label - self._state[data][cid] = comp_state - self._components[data]['main'].append(cid) + for cid in data.main_components: + comp_state = {} + comp_state['cid'] = cid + comp_state['label'] = cid.label + self._state[data][cid] = comp_state + self._components[data]['main'].append(cid) self._components[data]['derived'] = [] diff --git a/glue/viewers/custom/qt/custom_viewer.py b/glue/viewers/custom/qt/custom_viewer.py index 10abf5605..9aae73bef 100644 --- a/glue/viewers/custom/qt/custom_viewer.py +++ b/glue/viewers/custom/qt/custom_viewer.py @@ -1377,13 +1377,12 @@ def _list_components(self): """ Determine which components to list. - This can be overridden by subclassing to limit which components are visible to the user. """ comps = list(set([c for l in self.container.layers - for c in l.data.components if not c._hidden])) + for c in (l.data.main_components + l.data.derived_components)])) comps = sorted(comps, key=lambda x: x.label) return comps From 0e8b58263bf9accab44585f20bb8351f09154130 Mon Sep 17 00:00:00 2001 From: Thomas Robitaille Date: Sun, 25 Feb 2018 22:28:01 +0000 Subject: [PATCH 19/19] Remove leftover print statements --- glue/core/data.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/glue/core/data.py b/glue/core/data.py index d1a744885..598083230 100644 --- a/glue/core/data.py +++ b/glue/core/data.py @@ -458,10 +458,8 @@ def _set_externally_derivable_components(self, derivable_components): values are DerivedComponent instances which can be used to get the data. """ - print("_set_externally_derivable_components", len(derivable_components)) if len(self._externally_derivable_components) == 0 and len(derivable_components) == 0: return - print("HERE", self.hub) self._externally_derivable_components = derivable_components if self.hub: msg = ExternallyDerivableComponentsChangedMessage(self)