Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ag.rotate should rotate around selection center of mass/geometry #1022

Closed
kain88-de opened this issue Oct 11, 2016 · 4 comments
Closed

ag.rotate should rotate around selection center of mass/geometry #1022

kain88-de opened this issue Oct 11, 2016 · 4 comments

Comments

@kain88-de
Copy link
Member

Expected behaviour

If I do ag.rotate(R) I would expect that I can rotate the protein I selected around it's center of mass/geometry. I would propose we add a kwarg origin for this. This point determines the origin or the rotation and we default to the center of geometry (or lab for backwards compatibility)

def rotate(self, R, origin='cog'):
    if origin=='cog':
        point = self.center_of_geometry()
    elif origin=='com':
        point = self.center_of_mass()
    elif origin=='lab':
        point = np.zeros(3)

    self.translate(-point)
    # do rotation
    self.translate(point)

Actual behaviour

The center of the rotation is the center of the coordinate system. This means instead of just an internal rotation I can rotate protein in my lab system to another place.

Currently version of MDAnalysis:

(run python -c "import MDAnalysis as mda; print(mda.__version__)")
0.16.0dev

@kain88-de
Copy link
Member Author

We could also allow to give an arbitrary origin to this name selection

@richardjgowers
Copy link
Member

@kain88-de there is also currently rotateby which looks like it does something like what you want?

@kain88-de
Copy link
Member Author

But it doesn't accept a rotation/transformation matrix. Besides I spend today hours until I remembered this fact about rotate. Then that rotate and rotateby behave differently isn't good since one accepts a matrix and the other an axis and angle, plus origin of rotation, even though the docs say they do exactly the same thing.

@kain88-de
Copy link
Member Author

I noticed today (after around one hour of debugging) that this change broke alignto and AlignTraj when we align an a selection instead of the whole protein. The problem is that align.rotation_matrix returns a matrix that assume the rotation is done around a point centered at (0,0,0) in the lab-system. Which was the old behavior when rotate is used. Since this is a very subtle change which can break existing user code in non obvious ways, I would revert this change and have rotate use (0,0,0) as a center for the rotation again. This means that rotate and rotateby have slightly different default behavior but I rather live with that for historical reasons then break lots of code that is used.

I will also include a new alignment test that shows the failure this change introduced.

@kain88-de kain88-de reopened this Mar 12, 2017
kain88-de added a commit to kain88-de/mdanalysis that referenced this issue Mar 13, 2017
To not break a lot of old code restore the old behavior of rotate to
use (0, 0, 0) as center of the rotation.
kain88-de added a commit to kain88-de/mdanalysis that referenced this issue Mar 16, 2017
To not break a lot of old code restore the old behavior of rotate to
use (0, 0, 0) as center of the rotation.
orbeckst added a commit that referenced this issue Mar 16, 2017
Revert rotate default to before #1022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants