I think we currently have too many way of representing matrices in the IMP API. - Rotation3D - EulerAnglesZYZ - and EulerMatrix
The last doesn't make any sense as there is no reason to tie together two disparate representations. Given that one can create a Rotation3D from Euler angles I don't see any reason to expose the second. If we find rotational matrices are faster than quaternions for actually performing the rotations, then we should cache the matrix in Rotation3D rather than add another class.
Comments? Justifications for having many different versions?
1) I found problems with Rotation3D when designing project(). I coundn't make it work as I needed it. Hence the matrices. 2) EulerAnglesZYZ is a way of enforce the ZYZ convention when using EulerMatrixZYZ. Just a nice way of constructing a EulerMatrixZYZ whitout passing a Vector3D, as you suggested with SphericalCoords.
I definitively need the code.
Given that one can create a Rotation3D from Euler angles I don't see any > reason to expose the second.
Not entirely true. Right now you can create it for other conventions (I don't remember which ones, but at least two), not ZYZ. I mentioned at some point the need to establish a convention, or a general framework to treat conventions, comment that was ignored. If the functionality is there and works, why not expose it?
On May 4, 2009, at 5:54 PM, Javier Ángel Velázquez Muriel wrote:
> > 1) I found problems with Rotation3D when designing project(). I > coundn't make it work as I needed it. Hence the matrices. If there are problems/limitations with Rotation3D, we should fix them rather than add a new class.
> 2) EulerAnglesZYZ is a way of enforce the ZYZ convention when using > EulerMatrixZYZ. Just a nice way of constructing a EulerMatrixZYZ > whitout passing a Vector3D, as you suggested with SphericalCoords. We already have constructing a Rotation3D from your choice of Euler angles (and you can get a matrix from that if you want). So there isn't much reason to be passing them around at all.
> > I definitively need the code. Sure, the question is more whether it should be part of the API or in an internal header or .cpp file where it is used. So far I don't see anything that justifies creating an alternative rotation passing convention (which would require lots of functions to be eventually duplicated to support both).
2009/5/4 Daniel Russel drussel@gmail.com
> > On May 4, 2009, at 5:54 PM, Javier Ángel Velázquez Muriel wrote: > > >> 1) I found problems with Rotation3D when designing project(). I coundn't >> make it work as I needed it. Hence the matrices. >> > If there are problems/limitations with Rotation3D, we should fix them > rather than add a new class.
I agree.
> > > 2) EulerAnglesZYZ is a way of enforce the ZYZ convention when using >> EulerMatrixZYZ. Just a nice way of constructing a EulerMatrixZYZ whitout >> passing a Vector3D, as you suggested with SphericalCoords. >> > We already have constructing a Rotation3D from your choice of Euler angles > (and you can get a matrix from that if you want). So there isn't much reason > to be passing them around at all.
No, that is not true. Given that no convention was established, there is freedom to use anyone. I checked, and ZYZ was not in the code, so I added myself. This convention is common in EM (Rot, Tilt, Psi). I didn't pass anything around.
> > > >> I definitively need the code. >> > Sure, the question is more whether it should be part of the API or in an > internal header or .cpp file where it is used. So far I don't see anything > that justifies creating an alternative rotation passing convention (which > would require lots of functions to be eventually duplicated to support > both)._______________________________________________ >
I will be happy hearing about any improvement in the API. I'm worried though about project(). It should work at least as it is working now, and respecting my choice of angles and rotation interface requesting a matrix, because that function is at the core of my project and I want that performance. If it can be improved, I gladly will acknowledge any contribution. As long as the function is able to access what requires, I don't mind the way it is done. I can provide stringent tests that I decided not to finally put in actual tests.
> IMP-dev mailing list > IMP-dev@salilab.org > https://salilab.org/mailman/listinfo/imp-dev >
> We already have constructing a Rotation3D from your choice of Euler > angles (and you can get a matrix from that if you want). So there > isn't much reason to be passing them around at all. > > No, that is not true. Given that no convention was established, > there is freedom to use anyone. Yes, there is no convention for which types of Euler Angles to use. There is a convention that all rotations be represented using a Rotation3D (which can be created from many things).
> Sure, the question is more whether it should be part of the API or > in an internal header or .cpp file where it is used. So far I don't > see anything that justifies creating an alternative rotation passing > convention (which would require lots of functions to be eventually > duplicated to support > both)._______________________________________________ > > I will be happy hearing about any improvement in the API. I'm > worried though about project(). It should work at least as it is > working now, and respecting my choice of angles and rotation > interface requesting a matrix, because that function is at the core > of my project and I want that performance. If it can be improved, I > gladly will acknowledge any contribution. As long as the function is > able to access what requires, I don't mind the way it is done. I can > provide stringent tests that I decided not to finally put in actual > tests. Given a relatively narrow interface, performance is generally pretty easy to achieve given a benchmark to test changes. The narrow interface is important since it means that you don't have to improve up too much code (eg having several Rotation classes means improvements to one are not available to functions which use another).
As I mentioned, we can always add an internal cache of the matrix values in Rotation3D if we find that such speeds things up. My suspicion is that the compiler would cache the results for us if you are applying the rotations in an inner loop and that the memory accesses would overwhelm the reduced number of multiplications if it is not, but it is easy to test once a benchmark is set up. For this, a benchmark of just rotating a matrix of random numbers would probably do and be very easy to set up.
It seems to me the old interface with - Rotation3D which has a rotate function - functions to create Rotation3Ds from various Euler angles (including ZYZ, if it is not already there) - possible caching of the rotation matrix in Rotation3D if that does speed things up covers everything that is needed without the addition of any of the new classes and conflicting conventions. Is this right?
Then anyone can, upon reading Euler angle values (of whatever convention) create a Rotation3D. This Rotation3D can be used to perform rotations in an efficient manner. And we don't have to worry about some functions take one representation for rotations, and some another.
just one comment: quaternions are more accurate than matrix multiplications, and it is a common practice to use quaternions to apply multiple rotations - so i would prefer if the internal implementation of Rotation3D would stay as it is On May 4, 2009, at 5:30 PM, Daniel Russel wrote:
> I think we currently have too many way of representing matrices in > the IMP API. > - Rotation3D > - EulerAnglesZYZ > - and EulerMatrix > > The last doesn't make any sense as there is no reason to tie > together two disparate representations. Given that one can create a > Rotation3D from Euler angles I don't see any reason to expose the > second. If we find rotational matrices are faster than quaternions > for actually performing the rotations, then we should cache the > matrix in Rotation3D rather than add another class. > > Comments? Justifications for having many different versions? > _______________________________________________ > IMP-dev mailing list > IMP-dev@salilab.org > https://salilab.org/mailman/listinfo/imp-dev
I put a function to build the equivalent quaternion in EulerMatrixZYZ. More flexibility ....
2009/5/4 Keren Lasker kerenl@salilab.org
> just one comment: quaternions are more accurate than matrix > multiplications, and it is a common practice to use quaternions to apply > multiple rotations - so i would prefer if the internal implementation of > Rotation3D would stay as it is > > On May 4, 2009, at 5:30 PM, Daniel Russel wrote: > > I think we currently have too many way of representing matrices in the IMP >> API. >> - Rotation3D >> - EulerAnglesZYZ >> - and EulerMatrix >> >> The last doesn't make any sense as there is no reason to tie together two >> disparate representations. Given that one can create a Rotation3D from Euler >> angles I don't see any reason to expose the second. If we find rotational >> matrices are faster than quaternions for actually performing the rotations, >> then we should cache the matrix in Rotation3D rather than add another class. >> >> Comments? Justifications for having many different versions? >> _______________________________________________ >> IMP-dev mailing list >> IMP-dev@salilab.org >> https://salilab.org/mailman/listinfo/imp-dev >> > > _______________________________________________ > IMP-dev mailing list > IMP-dev@salilab.org > https://salilab.org/mailman/listinfo/imp-dev >
On May 4, 2009, at 6:09 PM, Keren Lasker wrote:
> just one comment: quaternions are more accurate than matrix > multiplications, and it is a common practice to use quaternions to > apply multiple rotations - so i would prefer if the internal > implementation of Rotation3D would stay as it is I'm not sure this is correct as rotation using a quaternion is more or less computing the rotation matrix and then applying it (hence it is slower than using a rotation matrix). Composing quaternions is much more stable than matrices (and the errors make much more sense).
participants (3)
-
Daniel Russel -
Javier Ángel Velázquez Muriel -
Keren Lasker