plane and axis centering transformations by davidercruz · Pull Request #1973 · MDAnalysis/mdanalysis

davidercruz · 2018-07-07T00:34:15Z

Adds plane and axis centering transformations

PR Checklist

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

…changelogs

coveralls · 2018-07-07T00:57:28Z

Coverage decreased (-0.03%) to 89.922% when pulling e066f17 on davidercruz:more-translations into 1a1b2f1 on MDAnalysis:develop.

jbarnoud

Oops! I forgot to click "submit". I am very sorry about that. It is great that Github saves pending reviews!!!!!

jbarnoud · 2018-07-07T07:48:35Z

package/MDAnalysis/transformations/translate.py

+
+    Returns
+    -------
+    :class:`~MDAnalysis.coordinates.base.Timestep` object


This line could be just MDAnalysis.coordinates.base.Timestep without the :class: and "object". See https://numpydoc.readthedocs.io/en/latest/format.html#docstring-standard.

jbarnoud · 2018-07-07T08:01:45Z

package/MDAnalysis/transformations/translate.py

+    is centered on an axis. This axis can only be parallel to the x, y or z planes. 
+
+    Example
+    -------


You need to tell what your example is doing, or I just see random code.

Also, the example is aimed at the user which is not really supposed to use the function directly on a TimeStep. Your example should probably use the transformation feature.

jbarnoud · 2018-07-07T08:08:47Z

package/MDAnalysis/transformations/translate.py

+            center = np.asarray([_origin[0], ag_center[1], _origin[2]], np.float32)
+        if axis == 'z':
+            center = np.asarray([_origin[0], _origin[1], ag_center[2]], np.float32)
+        vector = center - ag_center


Are we guaranteed that the dtype of vector will be float32?

center_of_mass and center_of_geometry output an array of dtype float32. I can typecast it again, but it isn't really necessary.

ps - unless someone changes the center_of functions in the future. If that changes than tests will likely fail.

There is no need to typecast again. Though, it would be good to have a test for each transformation to make sure the coordinates have the correct dtype.

jbarnoud · 2018-07-07T08:09:42Z

package/MDAnalysis/transformations/translate.py

+    ----------
+    ag: AtomGroup
+        atom group to be centered on the unit cell.
+    plane: str


'x', 'y', and 'z' are not planes so your explanation is unclear. I guess if I want to center on the 'XY' plane, I need to provide 'z'; but I am not really sure. This makes for an unclear API.

This is still an issue.

Centering on xz,xy and yz planes is not trivial. Since these planes cross more than 1 axis, the center of these planes is the center of the cross-section area formed by these planes in the unit cell.
Currently, the code only works with planes that are parallel to x=0, y=0 or z=0.

The way I see it, assuming a orthogonal box, centering on XY means you do not change the Z coordinate. It is always tricky to have a mental picture of an arbitrary triclinic box, but assuming the lower triangle of the box matrix is 0, then it should work the same. If the triangle is not 0, then it is trickier as you do have to change Z; but we can make sure it is not the case and fail if it is.

okay I see I was confused, sorry. XY is the same as z=0, XZ is y=0 and YZ is x=0. I was talking about non-perpendicular planes, sorry. I will simply change the strings to match.

jbarnoud · 2018-07-07T08:11:55Z

package/MDAnalysis/transformations/translate.py

-
+
+
+def center_in_plane(ag, plane, coordinate=0, origin=None, center='geometry', wrap=False):


Why defaulting 'origin' to None and not its value?

jbarnoud · 2018-07-07T08:12:53Z

package/MDAnalysis/transformations/translate.py

+
+    """
+
+    if isinstance(plane, string_types):


You do not need to test the type. You just need to test is it is not None and not in 'xyz'. In general, testing on type should be avoided. Python is keen on duck-typing. If I write an object that pretends to be 'x', you should not discriminate against it if you can avoid it.

jbarnoud · 2018-07-07T08:18:41Z

package/MDAnalysis/transformations/translate.py

+        if plane == 'z':
+            shift = _origin[2]+coordinate
+            position = [boxcenter[0], boxcenter[1], shift]
+        ts = center_in_box(ag, center=center, point=position, wrap=wrap)(ts)


You are doing for every frame all the checks that you removed from the wrapped function in center_in_box. This does not seem very efficient.

jbarnoud

Some more comments on top of the previous ones that are still valid.

jbarnoud · 2018-07-11T20:36:03Z

package/CHANGELOG


 Enhancements
-
+  * Added box/axis/plane centering trajectory transformations


Instead of adding this line to the changelog, you can update the other line about centering method by listing this PR next to the other one.

jbarnoud · 2018-07-11T20:36:40Z

package/MDAnalysis/transformations/translate.py

    Returns
    -------
-    :class:`~MDAnalysis.coordinates.base.Timestep` object
+    `~MDAnalysis.coordinates.base.Timestep`


Just the type, no back-tick nor tilde.

jbarnoud · 2018-07-11T20:40:10Z

package/MDAnalysis/transformations/translate.py

+    .. code-block:: python
+
+        ag = u.residues[1].atoms
+        ts = MDAnalysis.transformations.center_in_axis(ag,'x', [0,0,1], center='mass')(ts)


My previous comment on the example applies here as well.

jbarnoud · 2018-07-11T20:45:08Z

package/MDAnalysis/transformations/translate.py

+            center = np.asarray([_origin[0], ag_center[1], _origin[2]], np.float32)
+        if axis == 'z':
+            center = np.asarray([_origin[0], _origin[1], ag_center[2]], np.float32)
+        vector = center - ag_center


There is no need to typecast again. Though, it would be good to have a test for each transformation to make sure the coordinates have the correct dtype.

codecov · 2018-07-11T22:39:30Z

Codecov Report

Merging #1973 into develop will increase coverage by 0.03%.
The diff coverage is 97.95%.

@@             Coverage Diff             @@
##           develop    #1973      +/-   ##
===========================================
+ Coverage    89.44%   89.48%   +0.03%     
===========================================
  Files          157      157              
  Lines        18775    18857      +82     
  Branches      2713     2722       +9     
===========================================
+ Hits         16794    16874      +80     
- Misses        1378     1379       +1     
- Partials       603      604       +1

Impacted Files	Coverage Δ
package/MDAnalysis/transformations/__init__.py	`100% <100%> (ø)`	⬆️
package/MDAnalysis/transformations/translate.py	`98.33% <97.93%> (-1.67%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update abcdb1e...589ce9c. Read the comment docs.

jbarnoud · 2018-07-13T08:53:15Z

@davidercruz My comments on the API still stand:

Your functions are called center_* but do not move the atom group to the center of the box by default;
the name origin is confusing, especially next to center and the name of the functions;
x, y, and z are not planes, you cannot use them as values for the plane argument of center_to_plane

At this point, I think these are the main issues. The rest looks OK, but depends on the API.

For origin and center, I suggest to rename them center_from and center_to, respectively. The naming is not super consistent with the rest but you could change your other transformations to match that nomenclature. These names are likely not the best, but I think they are less confusing.

… changed some options names to something clearer; updated tests

jbarnoud

Could you:

remove the d argument. The name is bad (it is not explicit at all), and it triger a behaviour that is out of the scope of the function;
avoid the succesion of ifs with the dict trick I showed you;
rename center_from to center_to, one of my earlier comment was confusing.

jbarnoud

I have connection issues so I am reviewing from my phone. It is less than optimal so I expect I missed some stuff...

jbarnoud · 2018-07-21T06:27:50Z

package/MDAnalysis/transformations/translate.py


    def wrapped(ts):
-        if isinstance(center_from, string_types) and center_from == 'center':
+        if isinstance(center_to, string_types) and center_to == 'center':


You do not need to test for the type. Testing for the value is enough.

jbarnoud · 2018-07-21T06:29:42Z

package/MDAnalysis/transformations/translate.py

+        used to define the plane on which the given AtomGroup will be centered. Suported
+        planes are yz, xz and xy planes.
+    center_to: array-like or str
+        coordinate from which the axes are centered. Can be an array of three coordinate 


Coordinates to which...

jbarnoud · 2018-07-21T06:30:51Z

package/MDAnalysis/transformations/translate.py

+    """
+
+    pbc_arg = wrap
+    if plane is not None:


Can 'plane' be None? What would it mean?

jbarnoud · 2018-07-21T06:36:38Z

package/MDAnalysis/transformations/translate.py

+        if center_to != 'center':
+            _origin = center_to
+            position[plane] = _origin[plane]
+        vector = np.asarray(position, np.float32) - center_method()


Are you really working in 2D there? From what I read you are centering in the box, not the plane.

jbarnoud · 2018-07-21T06:38:27Z

package/MDAnalysis/transformations/translate.py

+                raise ValueError('{} is not a valid "center_to"'.format(center_to))
+        else:
+            center_to = np.asarray(center_to, np.float32)
+            if center_to.shape != (3, ) and center_to.shape != (1, 3):


Does it actually work with a (1, 3) vector ? Did you try?

kain88-de

Looks nice. I mainly would like the center calculations to make use of the flexibility in MDAnalysis that we expose in the encore module

kain88-de · 2018-07-21T11:31:16Z

package/MDAnalysis/transformations/translate.py

            raise ValueError('{} is not a valid point'.format(point))
    try:
-        if center == 'geometry':
+        if center_of == 'geometry':


why do we have center_of here all the time? Can't you make use of the generic center as well? it allows to center with respect to arbitrary weights. People will want to do this. Think of IDP proteins where you want to calculate a center with respect to the flexibility of residues.

You could have an argument weights='mass' like we have for the encore methods. The argument would accept mass and centroid as string arguments or an array-like object or None.

kain88-de · 2018-07-21T11:34:24Z

package/MDAnalysis/transformations/translate.py

+        axis = axes[axis]
+    if center_to is not None:
+        if isinstance(center_to, string_types):
+            if not center_to == 'center':


unnecessary check and nesting of if clauses. You can leave out the isinstance check in the first place. Here you also have an unnecessary nesting of the if clause. It would be better to write the two tests into the same if-clause.

kain88-de · 2018-07-21T11:38:44Z

package/MDAnalysis/transformations/translate.py

+        which sets [0, 0, 0] as the origin of the axis. 
+    center_of: str, optional
+        used to choose the method of centering on the given atom group. Can be 'geometry'
+        or 'mass'


I would like to be able to give this arbitrary weights to calculate the center. This is for all your methods in all PRs you have so far. It's a useful addition and makes MDAnalysis more flexible. I actually ran across an issue where I would like to calculate the center with respect to arbitrary weights several times. I'm not aware that any other tool besides MDAnalysis allows to do this.

kain88-de · 2018-07-21T11:39:43Z

package/MDAnalysis/transformations/translate.py

+    .. code-block:: python
+
+        ag = u.residues[1].atoms
+        transform = MDAnalysis.transformations.center_in_axis(ag, 'x', [0,0,1], center_of='mass')


mda.transformations....

…culation

…analysis into more-translations

kain88-de · 2018-07-22T13:07:57Z

looks like there are some python 2.7 specific errors left

davidercruz · 2018-07-22T13:31:53Z

apparently bytes from strings (b'xy' as plane for example) behave differently in python 2 and python 3. While in python 3 the functions fails as expected, in python 2 it doesn't. What should I do? Should I remove this type from the tests?

kain88-de · 2018-07-22T13:44:27Z

which line fails?

…

On Sun, Jul 22, 2018 at 3:31 PM Davide Cruz ***@***.***> wrote: apparently bytes from strings behave differently in python 2 and python 3. While in python 3 the functions fails as expected, in python 2 it doesn't. What should I do? Should I remove this type from the tests? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1973 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEGnVj7PmGZRuTQdhG0r9r2BXyoeTZK2ks5uJH7KgaJpZM4VGKdl> .

davidercruz · 2018-07-22T20:13:52Z

@kain88-de I think I had already faced this problem in the groupby PR months ago. Python 2 doesn't have a bytes type. So, while a bytes string is different from a regular string in python3, in python 2 it is the same.

kain88-de · 2018-07-23T07:54:38Z

package/MDAnalysis/transformations/translate.py

    except (KeyError, TypeError):
        raise ValueError('{} is not a valid plane'.format(plane))
-
-    if isinstance(center_to, string_types):


did you remove the import of string_types?

jbarnoud · 2018-08-02T11:53:05Z

@davidercruz Can yuu elaborate why you have to use such a weird workflow to reach your goal? You should only need

transformations = (mda.transformations.unwrap(prot))
                              mda.transformations.center_in_box(prot, wrap=False),)

Is your notebook visible somewhere? If no, could you share it as a github gist?

davidercruz · 2018-08-02T13:20:23Z

@jbarnoud That is fine for a protein in solution, in a trajectory without solvent.
But when you have a membrane protein, and the protein is off center, when you center the protein, part of the membrane will be outside the unit cell. That's why I do the wrap (to put everything back in the unit cell) and the unwrap of the whole system (ag).

jbarnoud · 2018-08-02T13:24:20Z

@davidercruz And what does the unwrap option of the centering methods (I did not have time to catch up yet).

davidercruz · 2018-08-02T13:28:39Z

@jbarnoud It runs make_whole for the atomgroup that needs to be centered. If #2012 is merged, make_whole will be called directly from atoms.center() just for the calculation, but without changes to the actual AtomGroup.

richardjgowers · 2018-08-02T13:31:29Z

What we need is a function which chooses the image closest to a given reference coordinate. Ie translate box lengths to minimise the distance from AG to point. This exists in the wrap method currently, but can be moved out.

…

On Thu, Aug 2, 2018 at 8:28 AM, Davide Cruz ***@***.***> wrote: @jbarnoud <https://github.com/jbarnoud> It runs make_whole for the atomgroup that needs to be centered. If #2012 <#2012> is merged, make_whole will be called directly from atoms.center() just for the calculation, but without changes to the actual AtomGroup. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1973 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AI0jBzZrghRZoIJ2QCzTWOpBtU-ZWxypks5uMv6LgaJpZM4VGKdl> .

jbarnoud · 2018-08-02T14:01:46Z

The unwrap argument is a good idea, though I would not encourage it in a transformation workflow as would will end up unwrapping the same molecules multiple times. For transformations, a function that puts things back in the expected image would be better. It would combine the two last steps in your workflow, hopefully in a most efficient way.

About the unwrap argument: what happens if I say warp=True, unwrap=True? At some point, somebody suggested having a pbc argument that could take 'no', 'wrap', or 'unwrap' as value. This could be an option.

davidercruz · 2018-08-02T14:04:23Z

@jbarnoud It raises a ValueError. My inital thought was to simply give priority to unwrap and if both were True, then wrap would become False, but now I think an error is better.
Should I added this to the docs?

davidercruz · 2018-08-02T14:50:17Z

@richardjgowers @jbarnoud
I completely forgot that ag.wrap(compound='fragments') existed. I can create a clean() transformation that does just this.

Thus the workflow could be

ag = u.atoms
prot = u.select_atoms('protein')
transformations = (mda.transformations.unwrap(ag))
                   mda.transformations.center_in_box(prot),
                   mda.transformations.clean(ag))

kain88-de · 2018-08-02T19:04:59Z

I completely forgot that ag.wrap(compound='fragments') existed. I can create a clean() transformation that does just this.

why not add the compound keyword to the wrap transformation? Can't you simply pass all kwargs along?

kain88-de · 2018-08-02T19:05:56Z

when you center the protein, part of the membrane will be outside the unit cell

A colleague has asked about exactly that problem and if the transformations can help with it.

orbeckst · 2018-12-14T20:46:53Z

Could someone knowledgeable with the discussion (@davidercruz @jbarnoud @richardjgowers @kain88-de ) please summarize with a view towards:

Is there a chance that this PR will get finished?
What are the major hurdles that would have to be overcome to make it work? (In case someone else might want to pick up the PR.)

Thanks.

jbarnoud · 2018-12-14T20:59:26Z

It will be finished. There is not much work left but I did not have time to finish the review so far. On 14 Dec 2018 21:46, Oliver Beckstein <notifications@github.com> wrote:Could someone knowledgeable with the discussion (@davidercruz @jbarnoud @richardjgowers @kain88-de ) please summarize with a view towards: Is there a chance that this PR will get finished?What are the major hurdles that would have to be overcome to make it work? (In case someone else might want to pick up the PR.) Thanks. —You are receiving this because you were mentioned.Reply to this email directly, view it on GitHub, or mute the thread.

orbeckst · 2018-12-14T21:01:45Z

Thanks!

…

On Dec 14, 2018, at 1:59 PM, Jonathan Barnoud ***@***.***> wrote: It will be finished. There is not much work left but I did not have time to finish the review so far.

davidercruz · 2018-12-21T14:53:25Z

As soon as @jbarnoud submits his review I'll make the changes necessary :D

orbeckst · 2019-04-12T20:15:03Z

@jbarnoud @richardjgowers @davidercruz any chance that this gets into 0.20.0 #2240 (as we had promised in the 0.19.x blog post)?

- fix #13 - needs MDAnalysisData >= 0.7.0 for the "membrane peptide" dataset - needs MDAnalysis >= 0.20.0 for the transformations (currently PRs MDAnalysis/mdanalysis#2038 , MDAnalysis/mdanalysis#1991 , and MDAnalysis/mdanalysis#1973 ) - updated text in notebook - adde a few more empty lines for clarity

orbeckst · 2019-06-04T01:16:22Z

@jbarnoud @kain88-de – sorry to ping you both but you both have open review requests on this PR. Can you provide a quick note if these requests still stand or if they have been addressed? Thanks!

hmacdope · 2023-11-08T20:34:16Z

Closing as stale, feel free to re-open if you want to continue.

added plane and axis centering transformations; added tests; updated …

d9bf82f

…changelogs

Merge branch 'develop' into more-translations

722c83b

jbarnoud reviewed Jul 11, 2018

View reviewed changes

better docs; all checks and calculations are done in center_in_plane

e066f17

jbarnoud requested changes Jul 11, 2018

View reviewed changes

dtype of coordinates is now tested; better docs; updated changelog

149cf23

davidercruz and others added 2 commits July 13, 2018 17:21

center_in_axis/plane now use the center of the unit cell as default ;…

c8a9f84

… changed some options names to something clearer; updated tests

Merge branch 'develop' into more-translations

05888fb

jbarnoud reviewed Jul 20, 2018

View reviewed changes

removed ifs; removed d arg

1a1ea9e

jbarnoud reviewed Jul 21, 2018

View reviewed changes

kain88-de requested changes Jul 21, 2018

View reviewed changes

davidercruz and others added 5 commits July 22, 2018 04:22

all center functions now accept weights ; changed center_in_plane cal…

cf645ea

…culation

Merge branch 'develop' into more-translations

b4001f1

code cleanup

24d9cc5

Merge branch 'more-translations' of https://github.com/davidercruz/md…

2865fa1

…analysis into more-translations

improved exceptions; improved tests coverage

e205dc4

davidercruz added 3 commits July 22, 2018 21:27

byte strings were causing tests to fail on python2.7

212ca07

more cleanup

2a46e04

code cleanup

0e8788a

kain88-de reviewed Jul 23, 2018

View reviewed changes

davidercruz added 2 commits July 23, 2018 15:49

removed deprecated imports

561f5db

updated docs

f7bce74

davidercruz and others added 2 commits August 3, 2018 18:52

updated tests

9f9d8d9

Merge branch 'develop' into more-translations

19b5ba3

davidercruz mentioned this pull request Oct 12, 2018

Blog post for on-the-fly transformations MDAnalysis/MDAnalysis.github.io#87

Merged

orbeckst mentioned this pull request Oct 12, 2018

blog post for 0.19.0 MDAnalysis/MDAnalysis.github.io#94

Closed

21 tasks

richardjgowers assigned richardjgowers and unassigned richardjgowers Dec 14, 2018

Merge branch 'develop' into more-translations

589ce9c

richardjgowers added the pbc_project label Jul 18, 2020

IAlibay added the close? Evaluate if issue/PR is stale and can be closed. label May 18, 2021

hmacdope added the stale label Nov 8, 2023

hmacdope closed this Nov 8, 2023

		No newline at end of file


		def center_in_plane(ag, plane, coordinate=0, origin=None, center='geometry', wrap=False):


		Enhancements

		* Added box/axis/plane centering trajectory transformations

Conversation

davidercruz commented Jul 7, 2018

PR Checklist

Uh oh!

coveralls commented Jul 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jbarnoud left a comment

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!

davidercruz Jul 11, 2018 • 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!

davidercruz Jul 13, 2018 • 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!

davidercruz Jul 13, 2018 • 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!

jbarnoud left a comment

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!

codecov bot commented Jul 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jbarnoud commented Jul 13, 2018

Uh oh!

jbarnoud left a comment

Choose a reason for hiding this comment

Uh oh!

jbarnoud left a comment

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!

kain88-de left a comment

Choose a reason for hiding this comment

Uh oh!

coveralls commented Jul 7, 2018 •

edited

Loading

davidercruz Jul 11, 2018 •

edited

Loading

davidercruz Jul 13, 2018 •

edited

Loading

davidercruz Jul 13, 2018 •

edited

Loading

codecov bot commented Jul 11, 2018 •

edited

Loading

davidercruz commented Jul 22, 2018 •

edited

Loading

davidercruz commented Aug 2, 2018 •

edited

Loading

davidercruz commented Aug 2, 2018 •

edited

Loading