Introduce operators to groups by jbarnoud · Pull Request #1239 · MDAnalysis/mdanalysis

jbarnoud · 2017-03-09T19:29:21Z

Fixes #726

Add operators, notably but not only set operators, to GroupBase. Change also the meaning of the == operator: previously, a == b was testing instance identoty (same as a is b), now two groups are equals if they are at the same level, from the same universe, and with the same elements in the same order.

I would like to change the behavior of the in operator. I am not sure of what it does now:

def __contains__(self, other):                                              
        if not other.level == self.level:                                       
            # maybe raise TypeError instead?                                    
            # eq method raises Error for wrong comparisons                      
            return False                                                        
        return other.ix in self._ix

I would like to be able to do things like atom in residue_group, or residue in segment.

PR Checklist

Tests?
Docs?
CHANGELOG updated?
Issue raised/referenced?

jbarnoud · 2017-03-09T19:32:23Z

I still need to write quite a lot of tests, but here is already the demo I am using to write the thing: https://gist.github.com/jbarnoud/80c13803b551ce4853eb1b5cb3d9e4a8

jbarnoud · 2017-03-09T21:58:33Z

For test purposes I would need a system with a ridiculously large number of segments. Is there a way to alter a universe to reasign segments? I could write a function that takes a universe and recreate a Topology object from it; but is there a simplest method? @richardjgowers @dotsdl

richardjgowers · 2017-03-10T10:50:07Z

@jbarnoud you should be able to use MDAnalysisTests.make_Universe. I think you can also make it produce a Universe with a specified shape, but it might not actually work... I really wouldn't use the TPR system, the TPR parser is one of our slowest iirc

richardjgowers · 2017-03-10T10:54:28Z

package/MDAnalysis/core/groups.py

-            Group with elements of `self` and `other` concatenated
-
-        """
+    def _get_other_index(self, other):


I don't like this method because it does two things at once (both the level check, as well as returned .ix). I do like centralising the level check so we get consistent error messages etc, but I'd prefer something like a decorator:

@only_same_level def __add__(self, other): # at this point, same_level check has happened @only_same_level def union(self, other): etc

Or if not a decorator, we could have a level check function inside each method

The decorator is a good idea indeed. I'll move to that.

richardjgowers · 2017-03-10T11:04:25Z

testsuite/MDAnalysisTests/core/test_groups.py

+
+    @staticmethod
+    def make_groups(u, level):
+        a = getattr(u, level)[10:100]


If you use the default make_Universe() system, you should be able to get away with using only 5 segments to test sets. There's nothing about the set operations which requires more than 5 objects

make_universe seems indeed to be the solution. I'll see how to reduce the number of element, I was just being greedy 😃

richardjgowers · 2017-03-10T11:12:58Z

testsuite/MDAnalysisTests/core/test_groups.py

+            yield self._test_union, a, b, c, d
+            yield self._test_intersection, a, b, c, d
+            yield self._test_difference, a, b, c, d
+            yield self._test_symmetric_difference, a, b, c, d


I know you've probably already thought of this, but we also need predictable behaviour when we work with duplicate items in a Group. Ie u.atoms[[0, 1, 2, 1, 2]] | u.atoms[[3, 4]] == u.atoms[[0, 1, 2, 3, 4]]

richardjgowers · 2017-03-10T11:14:27Z

package/MDAnalysis/core/groups.py

+    | ``s.issuperset(t)``           |            | test if all elements of    |
+    |                               |            | ``t`` are part of ``s``    |
+    +-------------------------------+------------+----------------------------+
+    | ``s.concatenate(t)``          | ``s + t``  | new Group with elements    |


Probably worth mentioning that this preserves order (and duplicates?), whereas .union will return a sorted unique version

I plan to split the table between set operations and not set operations.

richardjgowers · 2017-03-10T11:17:39Z

package/MDAnalysis/core/groups.py

+    | ``s.difference(t)``           | ``s - t``  | new Group with elements of |
+    |                               |            | ``s`` that are not in ``t``|
+    +-------------------------------+------------+----------------------------+
+    | ``s.symmetric_difference(t)`` | ``s ^ t``  | new Group with elements    |


It'd be cool if the symbol equivalent worked, you could override __xor__ etc

This is planned, indeed.

Done, right?

richardjgowers · 2017-03-10T11:36:13Z

We had some discussion about __contains__ briefly in #1163 where we decided to go for the explicit atom in segment.atoms rather than allowing atom in segment

jbarnoud · 2017-03-10T18:29:12Z

Time to step away for a bit. I think I dressed all of @richardjgowers comments:

I replaced _get_other_index by a decorator that only checks the level; I also created an ix_array property that returns the index as an array, even for components.
I overloaded the operators so we can make ag1 | ag2 or ag1 ^ ag2 like for sets.
I added tests for groups with duplicated and scrambled elements.
I now use make_Universe and less elements per groups.
I split the table in the doc to distinguish between set operators and the others.

In addition, I added the substract method that @orbeckst suggested in #726. It is mapped on the minus sign to be consistent with the plus sign that is mapped on the concatenate method. These two operator diverge from the behavior of sets, but make more sense like that in our specific context.

All the set operators are implemented.

I still have some testing to do. Especially, I need to test that levels cannot be mixed, and that the methods work when provided an elements. I also need to fix the error messages in the @_only_same_level decorator, and to clean up the documentation.

jbarnoud · 2017-03-10T18:51:58Z

I did not notice that ComponentBase had the comparison operators overridden already to compare levels. Maybe I should not override then in GroupBase to test subsets/supersets as it may be confusing. Any thought?

jbarnoud · 2017-03-12T08:30:01Z

I think it is ready for review. I'll rebase once it is ready to be merged.

richardjgowers · 2017-03-12T11:51:46Z

package/MDAnalysis/core/groups.py

+        the same order. Groups that are not at the same level or that belong
+        to different universe cannot be compared.
+        """
+        o_ix = other.ix_array


Here you probably need to use .ix otherwise Atoms evaluate equal to AtomGroups, eg u.atoms[1] == u.atoms[[1]]

substract -> subtract reworked subtract to use numpy set operations tried to use self.ix and self.universe rather than shortcutting

Suggested changes for PR #1239

substract -> subtract reworked subtract to use numpy set operations tried to use self.ix and self.universe rather than shortcutting

jbarnoud · 2017-03-13T09:24:53Z

I rebased, squashed, fixed the conflict, and force pushed.

richardjgowers · 2017-03-14T11:52:58Z

Ok this is a big change(tm) so we should get a +2, poke @MDAnalysis/coredevs

One thing I thought of looking at it today, we implement & ^ and |, which follow standard set behaviour, but the implementation of - doesn't. - is more like the opposite of +, which makes sense for MDA, but might trip someone up one day when one of the four set operation symbols doesn't do what they expect. So maybe remove the symbol versions and force people to use methods for & ^ and |

mnmelo · 2017-03-14T11:59:36Z

package/MDAnalysis/core/groups.py

+    |                               |            | common to ``s`` and ``t``  |
+    +-------------------------------+------------+----------------------------+
+    | ``s.difference(t)``           |            | new Group with elements of |
+    |                               |            | ``s`` that are not in ``t``|


Wouldn't s - t be the equivalent here?

Ah, I see why not: the treatment as set.

Nope, currently its:

ag1 = u.atoms[[1, 2, 1, 2, 3, 4]] ag2 = u.atoms[[3, 4, 5]] ag1 + ag2 == [1, 2, 1, 2, 3, 4, 3, 4, 5] ag1 & ag2 == [3, 4] ag1 | ag2 == [1, 2, 3, 4, 5] ag1 - ag2 == [1, 2, 1, 2] # ie take from ag1 unless in ag2

@jbarnoud Maybe we should make __sub__ == difference and leave subtract as method only access?

See subtraction discussion here: #726 (comment)

@richardjgowers I went back and forth to many time about it, I cannot have an opinion on that anymore. The current version follows @orbeckst suggestion.

I vote for the current status. Since + is not a set operator I think that it's more intuitive that - also isn't.

Alternatively, we could use another unused operator for difference() (such as %).
Not sure I love such a hackish idea, though.

I think we are converging on using - for the set difference.

jbarnoud · 2017-03-14T12:01:28Z

An other thing people use to set operators can trip on is that sets overload the comparison operators to test if a set is a subset/superset of an other. I chose not to do the same here, to avoid confusion with how the comparison operators are used for components in mda.

All these symbol ambiguity may be annoying for a user that often use sets and their operators. On the other hand, I mostly wrote these methods to be able to do this:

dppc_upper = upper_leaflet & dppc

orbeckst · 2017-03-14T16:05:07Z

Is '/' already used? '\' would be the "proper" set difference operator but that would be super-stupid to use in Python…

…

On 14 Mar, 2017, at 05:22, mnmelo ***@***.***> wrote: Alternatively, we could use another unused operator for difference() (such as %). Not sure I love such a hackish idea, though.

-- Oliver Beckstein * orbeckst@gmx.net skype: orbeckst * orbeckst@gmail.com

jbarnoud · 2017-03-14T20:03:50Z

@orbeckst / is free indeed. I do not see anything that would clash with using it for differences.

richardjgowers · 2017-03-14T20:40:32Z

We can't use symbols differently to the rest of Python, ie people shouldn't have to relearn the language because of our design choices. As far as I can see we can either do:

Only use + and - for the methods concatenate and subtract, set methods have to go through the methods. This means that + and - mostly work as opposites to each other (but don't imply a circular operation)
Use + for concatenate, and all set symbols for set operations, then subtract doesn't have a symbol associated with it. This means that & and - all work as expected according to the larger Python ecosystem

I think I'm in camp 2 so that @jbarnoud 's snippet of c = a & b just works

orbeckst · 2017-03-14T22:42:01Z

I am fine with keeping .subtract() just as a method. I don't really see it being used as much as +. We cannot really change + anymore and in any case, its use is consistent with how list concatenation works in Python.

~~I think I favor not overloading - in this case, though. Because at least then we do not induce the expectation that + is doing a set-add.~~

EDIT: I mean, assign - to the set difference, given that this is standard Python behaviors for set

So yes: Option 2 in #1239 (comment)

orbeckst

Looking really nice as far as I can tell.

I am just blocking it until the - operator is resolved.

orbeckst · 2017-03-14T22:46:46Z

package/MDAnalysis/core/groups.py

+    | ``s.concatenate(t)``          | ``s + t``  | new Group with elements    |
+    |                               |            | from ``s`` and from ``t``  |
+    +-------------------------------+------------+----------------------------+
+    | ``s.subtract(t)``             | ``s - t``  | new Group with elements    |


remove - operator for subtract(). (see discussion)

orbeckst · 2017-03-14T22:48:01Z

package/MDAnalysis/core/groups.py

+    |                               |            | contain the same elements  |
+    |                               |            | in the same order          |
+    +-------------------------------+------------+----------------------------+
+    | ``x in s``                    |            | test if ``x`` is part of   |


What does "part of s" mean? In exact contigious order or "any elements of x anywhere in s"?

Probably the former when I compare this to issubset() below but that could be made clearer.

I'll clarify this. It means component x is part of group s.

orbeckst · 2017-03-14T22:53:37Z

package/MDAnalysis/core/groups.py

+    |                               |            | common to ``s`` and ``t``  |
+    +-------------------------------+------------+----------------------------+
+    | ``s.difference(t)``           |            | new Group with elements of |
+    |                               |            | ``s`` that are not in ``t``|


I think we are converging on using - for the set difference.

orbeckst · 2017-03-14T22:56:48Z

package/MDAnalysis/core/groups.py

+        return other.ix in self.ix
+
+    def __sub__(self, other):
+        return self.subtract(other)


change to self.difference(other)

orbeckst · 2017-03-14T22:58:26Z

package/MDAnalysis/core/groups.py

        """
        return self._ix

+    @property


Why not just call it ix? The added _array sounds superfluous.

But I admit to my ignorance of the topology system: apparently there's also a ix already defined and that's not what you want? Under which circumstances is ix_array != ix? For Atom? Add some more docs. Perhaps add a SeeAlso or link to :meth:ix.

self.ix has a different meaning in ComponentBase and in GroupBase. For groups, self.ix == self.ix_array; both are the array of indices of the components. For components, instead, self.ix is the index of the component as an integer. self.ix_array gives a common interface between groups and components to get the index as an array without having to check for the type.

I'll add a word in the doc.

orbeckst · 2017-03-14T23:07:45Z

package/MDAnalysis/core/groups.py

+    @_only_same_level
+    def difference(self, other):
+        """Elements from this Group that do not appear in another
+


This is not the same as subtract.

One can also use -:

ag1 - ag2

or

ag1.difference(ag2)

orbeckst · 2017-03-14T23:08:19Z

package/MDAnalysis/core/groups.py

+    @_only_same_level
+    def symmetric_difference(self, other):
+        """Group of elements which are only in one of this Group or another
+


One can also use ^.

orbeckst · 2017-03-14T23:08:40Z

package/MDAnalysis/core/groups.py

+    @_only_same_level
+    def intersection(self, other):
+        """Group of elements which are in both this Group and another
+


One can also use &.

orbeckst · 2017-03-14T23:14:25Z

testsuite/MDAnalysisTests/core/test_groups.py

+        return a, b, c, d, e
+
+    @staticmethod
+    def make_groups_duplicated_and_scrumbled(u, level):


I like "scrumbled" (... even if you mean "scrambled") ;-)

Argghh. I wrote it with a french accent.

orbeckst · 2017-03-14T23:17:11Z

testsuite/MDAnalysisTests/core/test_groups.py

        assert_(str(sg) == '<SegmentGroup [<Segment 4AKE>]>')
+
+
+


Impressive battery of tests!

The only thing that seems to be missing is tests for the operator versions, -, &, |, ^.

Their binding is tested test_shortcut_overriding.

kain88-de · 2017-03-15T08:24:31Z

Does the change of behavior for - break any code inside of MDAnalysis or break existing user code in a way that no warnings/exceptions are shown and just the results change? I've recently noticed I made a change that silently changed results for alignto and I was lucky to notice it, see #1242.

richardjgowers · 2017-03-15T08:29:18Z

@kain88-de this is purely adding things, ie - et al don't exist yet, so should be fine

kain88-de · 2017-03-15T08:33:47Z

package/MDAnalysis/core/groups.py

 class GroupBase(_MutableBase):
    """Base class from which a Universe's Group class is built.

+	Instances of :class:`GroupBase` provide the following operations that


We should link to these docs from another place. People who use MDAnalysis will most likely not look at the docs for the low level classes like GroupBase. The set operations are a quite neat addition and others will likely want to use them. So should this be linked/show-cased in the normal doc rst files or should we just link to this from AtomGroup/ResidueGroup/SegmentGroup?

I was definitely going to include these operations in #1190 which is the tutorial/introduction docs. But yeah, adding them to GroupBase is effectively hiding them from users

Can we link to this from the high-level groups? I haven't done a lot of cross linking in rst docs. We could definitely add a See Also section to the high level groups.

jbarnoud · 2017-03-15T16:35:31Z

So the consensus is - for difference?

orbeckst

Minor comments but you addressed all my previous questions.

orbeckst · 2017-03-15T17:02:41Z

package/MDAnalysis/core/groups.py

-    | ``x in s``                    |            | test if ``x`` is part of   |
-    |                               |            | ``s``                      |
-    +-------------------------------+------------+----------------------------+
-    | ``x not in s``                |            | test if ``x`` is not part  |


Did you remove "x not in s"?

not in is not really an operator. It is just the logical opposite of in.

Is it worthwhile pointing out in the docs that this works as expected?

I am fine with leaving it out as you did, given that everything "just works"™ in a pythonic fashion.

Hopefully it "just works".

orbeckst · 2017-03-15T17:04:30Z

package/MDAnalysis/core/groups.py

+    AtomGroups can be compared and combined using group operators. For
+    instance, AtomGroups can be concatenated::
+
+        ag_concat = ag1 + ag2  # or ag_concat = ag1.concat(ag2)


I wouldn't put the alternative in a comment. Either a separate code line or mention it in text with a link to concat().

orbeckst · 2017-03-15T17:04:44Z

package/MDAnalysis/core/groups.py

+    treats the AtomGroups as sets, duplicates are removed from the resulting
+    group, and atoms are ordered::
+
+        ag_union = ag1 | ag2  # or ag_union = ag1.union(ag2)


same as above comment

orbeckst · 2017-03-15T17:05:00Z

package/MDAnalysis/core/groups.py

        new :class:`AtomGroup` for multiple matches.  This makes it
        difficult to use the feature consistently in scripts.

+    AtomGroups can be compared and combined using group operators. For


Very nice write-up!

orbeckst · 2017-03-15T17:05:14Z

package/MDAnalysis/core/groups.py

+    :meth:`difference` is the set version of :meth:`subtract`. The resulting
+    group is sorted and deduplicated.
+
+    All set methods are listed in the table bellow. These methods treats the


methods treat

(plural)

orbeckst · 2017-03-15T17:06:02Z

package/MDAnalysis/core/groups.py

+    | ``s.difference(t)``           | ``s - t``  | new Group with elements of |
+    |                               |            | ``s`` that are not in ``t``|
+    +-------------------------------+------------+----------------------------+
+    | ``s.symmetric_difference(t)`` | ``s ^ t``  | new Group with elements    |


Done, right?

jbarnoud · 2017-03-15T20:46:14Z

I started to actually use these operators for real applications. Here is just a little example because I relly enjoy the feature:

# Split lipids by leaflet and lipid type
lipids = saturated + unsaturated
# We assume that each lipid is a single residue with its atoms
# ordered from the head to the tails
top_head = sum(r.atoms[0] for r in lipids.residues)
top_head_z = top_head.positions[:, 2]
upper_mask = top_head_z > top_head_z.mean()
upper = lipids.residues[upper_mask].atoms
lower = lipids - upper
upper_saturated = upper & saturated
lower_saturated = lower & saturated
upper_unsaturated = upper & unsaturated
lower_unsaturated = lower & unsaturated

orbeckst · 2017-03-15T20:47:17Z

Blog post!!!

orbeckst · 2017-03-15T20:48:03Z

@jbarnoud can you do a final clean up of the history and then I am happy to merge (unless anyone else has any further comments).

richardjgowers · 2017-03-15T20:48:06Z

Haha yeah, blog it!

…

On Wed, 15 Mar 2017, 8:47 p.m. Oliver Beckstein, ***@***.***> wrote: Blog post!!! — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#1239 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AI0jBzCfsi8BXyZ91Wbl10wwb5uyoW8Sks5rmE5VgaJpZM4MYhkw> .

See #726 This commit introduce several method to the GroupBase class, including all the set operators, and a subtract method. Some operator symbols are overridden.

substract -> subtract reworked subtract to use numpy set operations tried to use self.ix and self.universe rather than shortcutting

jbarnoud · 2017-03-15T20:58:18Z

@orbeckst I reworked the history. There is now 4 commits instead on plenty. I did not squash everything together to avoid loosing track @richardjgowers' contribution.

orbeckst · 2017-03-15T21:09:13Z

Perfect! I'll just wait for travis, just to make extra sure.

jbarnoud added the Work in progress label Mar 9, 2017

richardjgowers reviewed Mar 10, 2017

View reviewed changes

jbarnoud changed the title ~~[WIP] Introduce operators to groups~~ Introduce operators to groups Mar 12, 2017

jbarnoud added needs review and removed Work in progress labels Mar 12, 2017

richardjgowers reviewed Mar 12, 2017

View reviewed changes

richardjgowers added a commit that referenced this pull request Mar 12, 2017

Suggested changes for PR #1239

7e96505

substract -> subtract reworked subtract to use numpy set operations tried to use self.ix and self.universe rather than shortcutting

jbarnoud added a commit that referenced this pull request Mar 12, 2017

Merge pull request #1241 from MDAnalysis/set-operators-RGreview

54a40f6

Suggested changes for PR #1239

jbarnoud force-pushed the set-operators branch from f12aff3 to 0a9ebbe Compare March 13, 2017 09:23

jbarnoud pushed a commit that referenced this pull request Mar 13, 2017

Suggested changes for PR #1239

0a9ebbe

substract -> subtract reworked subtract to use numpy set operations tried to use self.ix and self.universe rather than shortcutting

richardjgowers approved these changes Mar 13, 2017

View reviewed changes

mnmelo reviewed Mar 14, 2017

View reviewed changes

orbeckst requested changes Mar 14, 2017

View reviewed changes

kain88-de reviewed Mar 15, 2017

View reviewed changes

orbeckst approved these changes Mar 15, 2017

View reviewed changes

richardjgowers self-requested a review March 15, 2017 17:11

richardjgowers approved these changes Mar 15, 2017

View reviewed changes

jbarnoud and others added 4 commits March 15, 2017 21:53

Introduce operators to groups

8be7930

See #726 This commit introduce several method to the GroupBase class, including all the set operators, and a subtract method. Some operator symbols are overridden.

Suggested changes for PR #1239

0f687ec

substract -> subtract reworked subtract to use numpy set operations tried to use self.ix and self.universe rather than shortcutting

Updated docstrings for set operators

b7281dd

Polish set operators

04391b4

jbarnoud force-pushed the set-operators branch from 30f4f1a to 04391b4 Compare March 15, 2017 20:56

orbeckst self-assigned this Mar 15, 2017

orbeckst merged commit 1b165be into develop Mar 15, 2017

orbeckst deleted the set-operators branch March 15, 2017 23:25

Conversation

jbarnoud commented Mar 9, 2017 • edited by orbeckst Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

Uh oh!

jbarnoud commented Mar 9, 2017

Uh oh!

jbarnoud commented Mar 9, 2017

Uh oh!

richardjgowers commented Mar 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jbarnoud Mar 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

richardjgowers commented Mar 10, 2017

Uh oh!

jbarnoud commented Mar 10, 2017

Uh oh!

jbarnoud commented Mar 10, 2017

Uh oh!

jbarnoud commented Mar 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jbarnoud commented Mar 13, 2017

Uh oh!

richardjgowers commented Mar 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jbarnoud commented Mar 14, 2017

Uh oh!

orbeckst commented Mar 14, 2017 via email

Uh oh!

jbarnoud commented Mar 14, 2017

Uh oh!

richardjgowers commented Mar 14, 2017 • edited by orbeckst Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

orbeckst commented Mar 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

orbeckst left a comment

Choose a reason for hiding this comment

Uh oh!

jbarnoud commented Mar 9, 2017 •

edited by orbeckst

Loading

jbarnoud Mar 10, 2017 •

edited

Loading

richardjgowers commented Mar 14, 2017 •

edited by orbeckst

Loading

orbeckst commented Mar 14, 2017 •

edited

Loading