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

change return type of 'translate','rotate' to allow chaining of these functions. #1010

Closed
kain88-de opened this issue Sep 30, 2016 · 8 comments

Comments

@kain88-de
Copy link
Member

Expected behaviour

I wish I could do things like ag.translate(-ag.center_of_mass()).rotate(R).positions.
It is a nice chain of operations that is also very readable. Or another thing which I tried and failed is [ag.translate(-ag.center_of_mass()).positions for ag in list]

Actual behaviour

They fail because both translate and rotate don't return the modified AtomGroup but rather the object used to modify it.

Currently version of MDAnalysis:

(run python -c "import MDAnalysis as mda; print(mda.__version__)")
0.15.1-dev0

@kain88-de kain88-de added the API label Sep 30, 2016
@kain88-de
Copy link
Member Author

Btw we have to be very careful with such a change because it will likely break a lot of user code.

@jbarnoud
Copy link
Contributor

So, basically, ag.rotate returns its input argument? It would indeed be much more useful to return the modified AtomGroup.

@kain88-de
Copy link
Member Author

@jbarnoud yes.

@richardjgowers
Copy link
Member

From what I can see, they just return the transformation you applied to them, which is usually known as it was an argument....

Can this be done onto the 363 branch to avoid any regressions?

@kain88-de
Copy link
Member Author

Can this be done onto the 363 branch to avoid any regressions?

I think so. But I saying again that this change will likely break user code. So I would be careful with it.

@kain88-de
Copy link
Member Author

@orbeckst what do you think about this change. Your are mostly a reasonable user-voice if we should do an API-break.

BTW if we do this I would use one/two versions that displays a warning that the return type of these functions will change and that scripts shouldn't rely on it.

@orbeckst
Copy link
Member

orbeckst commented Oct 7, 2016

This is a good suggestion and makes more sense than the current behavior. (History: The reason for returning the argument was that the arguments can be processed, for instance, translate() can take a tuple of two AtomGroups or Atoms and then it computes the translation vector or rotateby() returns the rotation matrix, given the axis and the angle.) I doubt that the return argument was used very much so although it will break code and I don't think we can easily ten2eleven-fix it I don't think it will be an awful break. But if we want to do it then pre-0.16 is a good time!

@kain88-de
Copy link
Member Author

we can easily ten2eleven-fix it

We could add a warning but we can't fix any code.

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

4 participants