imp
imp copied to clipboard
salilab
problem with euler angles
The algebra test test_uniform_rotation.py fails occasionally. The failure seems real - I changed the test (develop hash 3b492c437a) so that it would always fail, simply by repeating the same test a 1000 times - it fails in 0.5% of cases. Can you give it a look to see if there's real reason to worry?
Should probably just remove the functions. They are numerically unstable and no one has any interest in fixing them.
good point!! So, the test for Euler angles (in test_uniform_rotations.py) fails in 1:200 cases. I now checked in something that will make it fail every time by repeating it 1000 times. Probably due to numerical stability. Who is in charge of em2d these days, and feels like fixing it with advice from Daniel / Ben, I would guess.
On Wed, Apr 24, 2013 at 3:01 PM, duhovka [email protected] wrote:
get_rotation_from_fixed_zyz is used in em2d
— Reply to this email directly or view it on GitHubhttps://github.com/salilab/imp/issues/303#issuecomment-16971816 .
Barak
it is not ZXZ, it is ZYZ :) this function is ok, I would just get rid of ZXZ functions. One euler angle convention is enough.
hmm do we have any test for ZXZ? (somehow the fact that one has poor numerical stability could raise a suspicion that the other doesn't too :)
On Wed, Apr 24, 2013 at 3:07 PM, duhovka [email protected] wrote:
it is not ZXZ, it is ZYZ :) this function is ok, I would just get rid of ZXZ functions. One euler angle convention is enough.
— Reply to this email directly or view it on GitHubhttps://github.com/salilab/imp/issues/303#issuecomment-16972413 .
Barak
I mean ZYZ
On Wed, Apr 24, 2013 at 3:08 PM, Barak Raveh [email protected] wrote:
hmm do we have any test for ZXZ? (somehow the fact that one has poor numerical stability could raise a suspicion that the other doesn't too :)
On Wed, Apr 24, 2013 at 3:07 PM, duhovka [email protected] wrote:
it is not ZXZ, it is ZYZ :) this function is ok, I would just get rid of ZXZ functions. One euler angle convention is enough.
— Reply to this email directly or view it on GitHubhttps://github.com/salilab/imp/issues/303#issuecomment-16972413 .
Barak
Barak
algebra/test/test_zyz.py don't know if it passes :)
On Wed, Apr 24, 2013 at 3:08 PM, barakr [email protected] wrote:
I mean ZYZ
On Wed, Apr 24, 2013 at 3:08 PM, Barak Raveh [email protected] wrote:
hmm do we have any test for ZXZ? (somehow the fact that one has poor numerical stability could raise a suspicion that the other doesn't too :)
On Wed, Apr 24, 2013 at 3:07 PM, duhovka [email protected] wrote:
it is not ZXZ, it is ZYZ :) this function is ok, I would just get rid of ZXZ functions. One euler angle convention is enough.
— Reply to this email directly or view it on GitHub< https://github.com/salilab/imp/issues/303#issuecomment-16972413> .
Barak
Barak
— Reply to this email directly or view it on GitHubhttps://github.com/salilab/imp/issues/303#issuecomment-16972614 .
no miracles here :) I checked in something to show test_zyz.py is also terribly unstable. When it fails it sais (not my doing!): "bug Daniel about getting a more stable implementation or restructure your code to stay with quaternions" :)
On Wed, Apr 24, 2013 at 3:22 PM, duhovka [email protected] wrote:
algebra/test/test_zyz.py don't know if it passes :)
On Wed, Apr 24, 2013 at 3:08 PM, barakr [email protected] wrote:
I mean ZYZ
On Wed, Apr 24, 2013 at 3:08 PM, Barak Raveh [email protected] wrote:
hmm do we have any test for ZXZ? (somehow the fact that one has poor numerical stability could raise a suspicion that the other doesn't too :)
On Wed, Apr 24, 2013 at 3:07 PM, duhovka [email protected] wrote:
it is not ZXZ, it is ZYZ :) this function is ok, I would just get rid of ZXZ functions. One euler angle convention is enough.
— Reply to this email directly or view it on GitHub< https://github.com/salilab/imp/issues/303#issuecomment-16972413> .
Barak
Barak
— Reply to this email directly or view it on GitHub< https://github.com/salilab/imp/issues/303#issuecomment-16972614> .
— Reply to this email directly or view it on GitHubhttps://github.com/salilab/imp/issues/303#issuecomment-16974341 .
Barak
at least the xyz test is working :)
On Wed, Apr 24, 2013 at 3:28 PM, barakr [email protected] wrote:
no miracles here :) I checked in something to show test_zyz.py is also terribly unstable. When it fails it sais (not my doing!): "bug Daniel about getting a more stable implementation or restructure your code to stay with quaternions" :)
On Wed, Apr 24, 2013 at 3:22 PM, duhovka [email protected] wrote:
algebra/test/test_zyz.py don't know if it passes :)
On Wed, Apr 24, 2013 at 3:08 PM, barakr [email protected] wrote:
I mean ZYZ
On Wed, Apr 24, 2013 at 3:08 PM, Barak Raveh [email protected] wrote:
hmm do we have any test for ZXZ? (somehow the fact that one has poor numerical stability could raise a suspicion that the other doesn't too :)
On Wed, Apr 24, 2013 at 3:07 PM, duhovka [email protected] wrote:
it is not ZXZ, it is ZYZ :) this function is ok, I would just get rid of ZXZ functions. One euler angle convention is enough.
— Reply to this email directly or view it on GitHub< https://github.com/salilab/imp/issues/303#issuecomment-16972413> .
Barak
Barak
— Reply to this email directly or view it on GitHub< https://github.com/salilab/imp/issues/303#issuecomment-16972614> .
— Reply to this email directly or view it on GitHub< https://github.com/salilab/imp/issues/303#issuecomment-16974341> .
Barak
— Reply to this email directly or view it on GitHubhttps://github.com/salilab/imp/issues/303#issuecomment-16975135 .
Euler angles are intrinsically numerically unstable which is why we use quaternions. As it happens, one of the conversion functions is particularly bad as it involves lots of divisions by things that could be 0. One could check for each one and do an alternate calculation, but that would result in a function with rather complicated control flow (and which is little used). Based on conversations with Javi, he was just using them to process user input, where it doesn't matter as much.
I would suggest removing (and, of course, sticking versions in where they are used in em2d) them unless someone wants to volunteer to clean them up. But I think we have better things to clean :-)
On Apr 24, 2013, at 3:08 PM, barakr [email protected] wrote:
hmm do we have any test for ZXZ? (somehow the fact that one has poor numerical stability could raise a suspicion that the other doesn't too :)
On Wed, Apr 24, 2013 at 3:07 PM, duhovka [email protected] wrote:
it is not ZXZ, it is ZYZ :) this function is ok, I would just get rid of ZXZ functions. One euler angle convention is enough.
— Reply to this email directly or view it on GitHubhttps://github.com/salilab/imp/issues/303#issuecomment-16972413 .
Barak — Reply to this email directly or view it on GitHub.
we can remove only the conversion to Euler angles. I think they are not used (need to check). Conversion from Euler angles to quaternion does not have any problematic divisions.
On Wed, Apr 24, 2013 at 4:03 PM, drussel [email protected] wrote:
Euler angles are intrinsically numerically unstable which is why we use quaternions. As it happens, one of the conversion functions is particularly bad as it involves lots of divisions by things that could be 0. One could check for each one and do an alternate calculation, but that would result in a function with rather complicated control flow (and which is little used). Based on conversations with Javi, he was just using them to process user input, where it doesn't matter as much.
I would suggest removing (and, of course, sticking versions in where they are used in em2d) them unless someone wants to volunteer to clean them up. But I think we have better things to clean :-)
On Apr 24, 2013, at 3:08 PM, barakr [email protected] wrote:
hmm do we have any test for ZXZ? (somehow the fact that one has poor numerical stability could raise a suspicion that the other doesn't too :)
On Wed, Apr 24, 2013 at 3:07 PM, duhovka [email protected] wrote:
it is not ZXZ, it is ZYZ :) this function is ok, I would just get rid of ZXZ functions. One euler angle convention is enough.
— Reply to this email directly or view it on GitHub< https://github.com/salilab/imp/issues/303#issuecomment-16972413> .
Barak — Reply to this email directly or view it on GitHub.
— Reply to this email directly or view it on GitHubhttps://github.com/salilab/imp/issues/303#issuecomment-16977933 .
sounds good to me (assuming the conversions to are not really used and em2d would still be functional :)
On Wed, Apr 24, 2013 at 4:20 PM, duhovka [email protected] wrote:
we can remove only the conversion to Euler angles. I think they are not used (need to check). Conversion from Euler angles to quaternion does not have any problematic divisions.
On Wed, Apr 24, 2013 at 4:03 PM, drussel [email protected] wrote:
Euler angles are intrinsically numerically unstable which is why we use quaternions. As it happens, one of the conversion functions is particularly bad as it involves lots of divisions by things that could be 0. One could check for each one and do an alternate calculation, but that would result in a function with rather complicated control flow (and which is little used). Based on conversations with Javi, he was just using them to process user input, where it doesn't matter as much.
I would suggest removing (and, of course, sticking versions in where they are used in em2d) them unless someone wants to volunteer to clean them up. But I think we have better things to clean :-)
On Apr 24, 2013, at 3:08 PM, barakr [email protected] wrote:
hmm do we have any test for ZXZ? (somehow the fact that one has poor numerical stability could raise a suspicion that the other doesn't too :)
On Wed, Apr 24, 2013 at 3:07 PM, duhovka [email protected] wrote:
it is not ZXZ, it is ZYZ :) this function is ok, I would just get rid of ZXZ functions. One euler angle convention is enough.
— Reply to this email directly or view it on GitHub< https://github.com/salilab/imp/issues/303#issuecomment-16972413> .
Barak — Reply to this email directly or view it on GitHub.
— Reply to this email directly or view it on GitHub< https://github.com/salilab/imp/issues/303#issuecomment-16977933> .
— Reply to this email directly or view it on GitHubhttps://github.com/salilab/imp/issues/303#issuecomment-16978518 .
Barak
I falsely disabled the failed tests (test_zyz ; test_uniform_rotation) until somebody wished to handle this issue (while maintaining Javi's code functional).