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

disallow passing atomgroups to rotateby and translate #1025

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

disallow passing atomgroups to rotateby and translate #1025

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

Comments

@kain88-de
Copy link
Member

Related to #1022, #1010

So I'm working on getting the rotate. rotateby, translate and transform functions fixed up in the new topology and unify their behavior. I'm wondering now what the use case is for allowing translate and rotateby to accept a tuple of atomgroups. I myself would prefer to be more explicit and disallow the tuples. See this for translate

# what is allowed currently
ag.translate((sel1, sel2))
# vs. what I would use and prefer
ag.translate(sel1.centroid() - sel2.centroid())

For me the second example is better since the explicit about the translation vector. It is very easy now to change this to another difference vector like from the center_of_mass with search&replace in an editor. Yes it's more typing but it would help reading the analysis code later on since less magic happens.

For rotateby the API has more issues that people can easily trip over. This is because the point of rotation is chosen depending if an axis is provided or a tuple of atomgroups. I'm also absolutely clueless how the tuple of atomgroups could be usefil for rotateby.

@orbeckst
Copy link
Member

I can get behind simplifying this API along the lines of "explicit is better than implicit".

(We just need to make sure that we rip this one big band-aid off in one go...)

@dotsdl
Copy link
Member

dotsdl commented Oct 13, 2016

Meant to put this here; oops.

Related: I tried to deprecate this behavior in the last release, but there was some argument against this. I'm partial to your view, though, that robust simplicity is better than convenient but confusing complexity. See the discussion on #698.

@orbeckst
Copy link
Member

Btw, for rotateby I kind of liked the use of the tuple because it allowed you to directly describe rotations in terms of atoms (think rotating around a bond) and the behavior with picking the point on the axis from the first member of the tuple made some sense – at least I thought so ;-).

But yes, it's confusing, and probably not used very much, so do simplify things!

@orbeckst orbeckst removed the question label Oct 13, 2016
@orbeckst
Copy link
Member

There are probably enough people in favor to simplify along the lines of @kain88-de 's proposal (and I am reverting my originaly dissenting #698 (comment) because (1) it seems a feature that's not really proven itself very useful, (2) it makes it harder to achieve a consistent API, and (3) we are breaking everything anyway, so now is a good time).

Without tuples we also don't need to accept AtomGroups or Atoms so this will also simplify the code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants