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

Combine rotation commands #159

Closed
ctk3b opened this issue Dec 8, 2015 · 8 comments
Closed

Combine rotation commands #159

ctk3b opened this issue Dec 8, 2015 · 8 comments

Comments

@ctk3b
Copy link
Member

ctk3b commented Dec 8, 2015

It might be more elegant to have just two rotation commands:

rotate(compound, thetas) and rotate_around_axis(compound, thetas)

where thetas is an array of 3 angles.

@sallai
Copy link
Member

sallai commented Dec 9, 2015

I had thought about that, too, but I decided not to. The problem is that
rotations around different axes are not commutable operations. It is a very
rare case that the user first wants to rotate around the x axis, then
around the y, and finally around the z.

On Tue, Dec 8, 2015 at 11:01 AM Christoph Klein notifications@github.com
wrote:

It might be more elegant to have just two rotation commands:

rotate(compound, thetas) and rotate_around_axis(compound, thetas)

where thetas is an array of 3 angles.


Reply to this email directly or view it on GitHub
#159.

@ctk3b
Copy link
Member Author

ctk3b commented Dec 9, 2015

Hmm yeah didn't quite think that through.

@ctk3b ctk3b closed this as completed Dec 9, 2015
@chrisiacovella
Copy link
Contributor

You can still use the rotate command to just rotate about one axis at a time if that were for some reason useful.

@sallai
Copy link
Member

sallai commented Dec 9, 2015

That is true. But still, I think that having the name of the method
explicitly describe what axis we want to rotate around would be a bit more
user friendly:

rotate_around_z(compound, np.pi/4)
rotate(compoud, [0, 0, np.pi/4])

On Wed, Dec 9, 2015 at 9:41 AM chrisiacovella notifications@github.com
wrote:

You can still use the rotate command to just rotate about one axis at a
time if that were for some reason useful.


Reply to this email directly or view it on GitHub
#159 (comment).

@ctk3b
Copy link
Member Author

ctk3b commented Dec 9, 2015

We may want to come up with a better name than revolve_around_x for rotating in place though since that sounds like it does the same thing. Any ideas?

I might even lean towards swapping the names or perhaps "spin"?

@chrisiacovella
Copy link
Contributor

A spin operator sounds useful, since we often use that for initializing monolayers/bilayers (e.g., spin the molecule around it's long axis to randomize orientations). So to differentiate, spin could be a 2d operation, rotate is the 3d rotation matrix.

@sallai
Copy link
Member

sallai commented Dec 9, 2015

Yes, I like spin.

On Wed, Dec 9, 2015 at 1:01 PM chrisiacovella notifications@github.com
wrote:

A spin operator sounds useful, since we often use that for initializing
monolayers/bilayers (e.g., spin the molecule around it's long axis to
randomize orientations). I think that is better than calling it rotate,
since we can say rotate is the 3d rotation matrix.


Reply to this email directly or view it on GitHub
#159 (comment).

@chrisiacovella
Copy link
Contributor

This might be useful for the forcefield fitting stuff, too. For instance, if we want to calculate the dihedral, we'll spin around a specific bond.

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

3 participants