corrected rotateby transformation calculation by davidercruz · Pull Request #2000 · MDAnalysis/mdanalysis

davidercruz · 2018-07-21T01:47:52Z

Changes made in this Pull Request:

when rotating a given atomgroup from a different point than the origin, the rotation calculation is wrong

PR Checklist

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

codecov · 2018-07-21T02:35:08Z

Codecov Report

Merging #2000 into develop will increase coverage by 0.04%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           develop    #2000      +/-   ##
===========================================
+ Coverage    88.48%   88.52%   +0.04%     
===========================================
  Files          142      143       +1     
  Lines        17203    17259      +56     
  Branches      2635     2644       +9     
===========================================
+ Hits         15222    15279      +57     
+ Misses        1385     1383       -2     
- Partials       596      597       +1

Impacted Files	Coverage Δ
package/MDAnalysis/transformations/rotate.py	`100% <100%> (ø)`	⬆️
package/MDAnalysis/analysis/dihedrals.py	`97.82% <0%> (ø)`
package/MDAnalysis/lib/util.py	`86.86% <0%> (+0.33%)`	⬆️

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 238904d...e60b268. Read the comment docs.

kain88-de · 2018-07-21T04:16:12Z

testsuite/MDAnalysisTests/transformations/test_rotate.py

+    matrix = rotation_matrix(np.deg2rad(angle), vector, center_pos)
+    rotation = matrix[:3, :3]
+    translation = matrix[:3, 3]
+    ref.positions = np.dot(ref.positions, rotation) + translation


Can you use ag.transform(matrix) for the test. It’s an atom group method that accepts the 4x4 matrix for translation and rotation.

You're right. I used/looked at it in another transform (the fitting one), I thought " I could use this in the rotation transformation. Then I completely forgot.

I have a question though. Why do we use the inverse of the rotation matrix (the transpose) in atoms.transform and atoms.rotate? Is it related to the direction of the rotation (clockwise or counter-cockwise)?

Really it does that? I never use the combined method much myself. Did you check visually if the alignment is correct like this now or before?

kain88-de · 2018-07-22T12:50:28Z

package/MDAnalysis/transformations/rotate.py

+        If a float array of the same length as `ag` is provided, use each element of
+        the `array_like` as a weight for the corresponding atom in `ag`. Default is 
+        None.
    wrap: bool, optional


shouldn't this follow the atomgroup convention to call it pbc?

@richardjgowers proposed the change to wrap in #1946. The argument is explained in #1185 (comment) and I agree with it. Since there's no code built upon these transformations yet, there's no risk of breaking things.

yes, there is a risk of breaking user expectation. As a newbie using MDAnalysis I would be confusing why the transformation has a wrap argument and the corresponding atomgroup method a pbc argument. This is not an intuitive design.

Besides the comment also mentions to deprecate the pbc keyword argument. Is anyone on that yet?

@davidercruz did you start work on introducing the wrap keyword in the other atomgroup functions?

@kain88-de not yet

Yeah sorry, I didn't want to propagate the pbc keyword any more. I've started looking at the unwrap keyword. Adding the wrap keyword should just be deprecating pbc and aliasing it to wrap

kain88-de · 2018-07-22T12:52:03Z

testsuite/MDAnalysisTests/transformations/test_rotate.py

+    matrix = rotation_matrix(np.deg2rad(angle), vector, center_pos)
+    rotation = matrix[:3, :3]
+    translation = matrix[:3, 3]
+    ref.positions = np.dot(ref.positions, rotation) + translation


Really it does that? I never use the combined method much myself. Did you check visually if the alignment is correct like this now or before?

davidercruz · 2018-07-22T13:07:15Z

@kain88-de In theory, using the transposed of the matrix for a 45 degree rotation would result in a -45 degree rotation. There must be a reason for this being the default behaviour. I don't have a molecular viewer in this computer, but I'll try it tomorrow at work to see the difference.
Anyway, I have changed rotateby to be consistent with what's already implemented in the API

kain88-de · 2018-07-22T13:09:03Z

you can install vmd or nglview on your computer. They are both freely available

…

On Sun, Jul 22, 2018 at 3:07 PM Davide Cruz ***@***.***> wrote: @kain88-de <https://github.com/kain88-de> In theory, using the transposed of the matrix for a 45 degree rotation would result in a -45 degree rotation. There must be a reason for this being the default behaviour. I don't have a molecular viewer in this computer, but I'll try it tomorrow at work to see the difference. Anyway, I have changed rotateby to be consistent with what's already implemented in the API — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2000 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEGnVqdeF05VrZG_HgNcGqylBuj-RZUqks5uJHkEgaJpZM4VZcZl> .

davidercruz · 2018-07-22T21:31:08Z

@kain88-de I tested it on a molecule. I understand now. The rotations follow the right-hand rule.

orbeckst · 2018-07-28T01:30:29Z

package/MDAnalysis/transformations/rotate.py

-    center: str, optional
-        used to choose the method of centering on the given atom group. Can be 'geometry'
-        or 'mass'
+        use the eighted center of an AtomGroup as the point from where the rotation axis


orbeckst · 2018-07-28T01:39:54Z

General comment to @davidercruz : If you find an actual bug such as this one then please first raise an actual issue and then put in the PR. Fixing the bug is awesome but for keeping track of serious issues, it is much more convenient to have an actual issue to refer to (e.g., in discussions and in the CHANGELOG) – An issue is preferred over a PR number because one might actually need multiple PRs for fixing a complex issuesand conceptually it makes more sense to re-open an issue if necessary than to re-open a PR.

You can also raise an issue after the PR and edit the PR to refer to it...

kain88-de · 2018-07-28T12:16:48Z

If you find an actual bug such as this one then please first raise an actual issue and then put in the PR.

even better is to fix it in a separate PR. It gives more overview and less code to review. This makes the whole process easier for us and faster for you!

kain88-de

There are some issues left with the documentation. Did you also render it locally and check that it looks OK?

kain88-de · 2018-07-28T12:20:27Z

package/MDAnalysis/transformations/rotate.py

@@ -69,11 +76,14 @@ def rotateby(angle, direction, point=None, center="geometry", wrap=False, ag=Non
        vector that will define the direction of a custom axis of rotation from the


I would mention the expected shapes already in the docs.

kain88-de · 2018-07-28T12:20:34Z

package/MDAnalysis/transformations/rotate.py

    angle = np.deg2rad(angle)
+    try:
+        direction = np.asarray(direction, np.float32)
+        if direction.shape != (3, ) and direction.shape != (1, 3):


did you check the (1, 3) shape? I can't see it in the tests.

kain88-de · 2018-07-28T12:21:42Z

package/MDAnalysis/transformations/rotate.py

@@ -69,11 +76,14 @@ def rotateby(angle, direction, point=None, center="geometry", wrap=False, ag=Non
        vector that will define the direction of a custom axis of rotation from the
        provided point.
    ag: AtomGroup, optional


what happens if this is None? I find no mention in the documentation

davidercruz · 2018-07-30T16:31:28Z

@kain88-de Yes, I built the docs using sphinx and it looks ok now. I also updated the docs of my other PRs.

kain88-de · 2018-07-30T17:30:18Z

I think @jbarnoud should be back to give this a final look

fixed issue with rotation from non-origin point calculation

000f1e7

kain88-de reviewed Jul 21, 2018

View reviewed changes

davidercruz added 4 commits July 22, 2018 00:29

rotaateby now behaves as rotate, updated tests to use transform(matrix)

4f23420

rotateby now accepts weights

6cffae9

fixed typos

9ba25f1

code cleanup

9e5db30

kain88-de reviewed Jul 22, 2018

View reviewed changes

added checks for direction ; added tests for direction

f8d49fb

orbeckst reviewed Jul 28, 2018

View reviewed changes

kain88-de requested changes Jul 28, 2018

View reviewed changes

Improved docs; test point and vector in 2 shapes

e60b268

kain88-de approved these changes Jul 30, 2018

View reviewed changes

jbarnoud mentioned this pull request Aug 3, 2018

Inconsistent assignation behavior of the memory reader #2018

Closed

jbarnoud approved these changes Aug 5, 2018

View reviewed changes

jbarnoud added this to the 0.19.0 milestone Aug 5, 2018

jbarnoud self-assigned this Aug 5, 2018

jbarnoud added the Component-Transformations On-the-fly transformations label Aug 5, 2018

jbarnoud merged commit bf424a5 into MDAnalysis:develop Aug 5, 2018

		@@ -69,11 +76,14 @@ def rotateby(angle, direction, point=None, center="geometry", wrap=False, ag=Non
		vector that will define the direction of a custom axis of rotation from the

Conversation

davidercruz commented Jul 21, 2018

PR Checklist

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

kain88-de Jul 21, 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!

davidercruz Jul 21, 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!

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 commented Jul 22, 2018

Uh oh!

kain88-de commented Jul 22, 2018 via email

Uh oh!

davidercruz commented Jul 22, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

kain88-de commented Jul 28, 2018

Uh oh!

kain88-de 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!

davidercruz commented Jul 30, 2018

Uh oh!

kain88-de commented Jul 30, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov bot commented Jul 21, 2018 •

edited

Loading

kain88-de Jul 21, 2018 •

edited

Loading

davidercruz Jul 21, 2018 •

edited

Loading

orbeckst commented Jul 28, 2018 •

edited

Loading