-
Notifications
You must be signed in to change notification settings - Fork 311
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
[SofaKernel] FIX bug in toEulerVector #399
Conversation
Thank you for your pull request! |
Thanks for your PR @raffaellatrivisonne :-) |
[ci-build] |
@raffaellatrivisonne could you fix the build failure please? |
[ci-build] |
The function asin is defined in [-1,1]. |
From SOFA-dev meeting: |
I think I get your point but it adds an another parameter that changes nothing in practice. Here the point is to cut everything outside the bounds. With your suggestions, who will test the sensibility of this epsilon parameter ? It is not acceptable to have a number outside these bounds since asin is not defined at all |
const double epsilon = 1e-15;
Real y = Real(2.)*(q[3]*q[1] - q[2]*q[0]);
if( std::abs( double(y) ) - 1.0 > epsilon )
{
msg_error("Quat") << "Unexpectedly out of bounds argument for asin: " << y << msgendl;
} or const double epsilon = 1e-15;
Real y = Real(2.)*(q[3]*q[1] - q[2]*q[0]);
if( std::abs( double(y) ) - 1.0 > epsilon )
{
Real force_round = std::max( Real(-1.0), std::min(Real(1.0), y) );
msg_warning("Quat") << "Unexpectedly out of bounds argument for asin: " << y
<< "Force rounding to " << force_round << msgendl;
y = force_round;
} |
Why 1e-15 ? why not 1e-11 ? I guess Raphaella's number was just an example. I completely disagree with this idea. Introducing a new random chosen very locally defined epsilon is really a bad idea. But if it a command from the consortium guy's to close this pr I have to bow |
There is no command and I am far to pretend knowing enough how things are done in SOFA to claim my proposal is what has to be done. It is just a proposal discussed during last SOFA-dev meeting. |
Well, the message sounded like "you have to add an epsilon value" 🙄 . I let @raffaellatrivisonne do her job here for now on... good luck |
Hi everybody, |
Just passing by, I do not want to raise a flame war but actuall asin method has some documentation,
Then looking at This is probably not relevant here, since I presume the checks are there because in theory when a quaternion is normalized the value of |
Thanks for the explanations @raffaellatrivisonne and @fjourdes, let's merge then ! :-) |
[ci-build] |
Do someone knows why do tests are failing ? (In QuaterTest) |
I'm taking a look. |
I'm trying to debug to see where it fails, but it will take a little bit more than expected. |
So if I understood well we have 2 problems here:
|
The inital problem is the vector (that in my screenshot I call "q0 euler") which has its second component which is NaN.
But definetively EXPECT_EQ(p0,p1) should not pass with nan values for p1 |
maybe I am stating something already well known, but with c++11 there are some built in functions that can help to test floating point arithmetic.
|
Hi everybody, I'm trying to work on this PR, but I'm quite busy with my PhD in this moment and I don't think I will be able to finish it within a short delay. |
Hey, I just cleaned this oooooold PR. |
This PR:
Reviewers will merge only if all these checks are true.