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

Merged
merged 4 commits into from Sep 12, 2018

Conversation

8 participants
@raffaellatrivisonne
Copy link
Contributor

raffaellatrivisonne commented Sep 14, 2017


This PR:

  • builds with SUCCESS for all platforms on the CI.
  • does not generate new warnings.
  • does not generate new unit test failures.
  • does not generate new scene test failures.
  • does not break API compatibility.
  • is more than 1 week old (or has fast-merge label).

Reviewers will merge only if all these checks are true.

@sofabot

This comment has been minimized.

Copy link
Collaborator

sofabot commented Sep 14, 2017

Thank you for your pull request!
Someone will soon check it and start the builds.

@guparan

This comment has been minimized.

Copy link
Member

guparan commented Sep 15, 2017

Thanks for your PR @raffaellatrivisonne :-)
Could you add a small description explaining the error and your fix please?
[ci-build]

@damienmarchal

This comment has been minimized.

Copy link
Contributor

damienmarchal commented Sep 20, 2017

[ci-build]

@guparan

This comment has been minimized.

Copy link
Member

guparan commented Sep 25, 2017

@raffaellatrivisonne could you fix the build failure please?

@damienmarchal

This comment has been minimized.

Copy link
Contributor

damienmarchal commented Oct 10, 2017

[ci-build]

@raffaellatrivisonne

This comment has been minimized.

Copy link
Contributor Author

raffaellatrivisonne commented Oct 11, 2017

The function asin is defined in [-1,1].
The fix prevents NAN when the argument is slightly >1 due to numerical errors (1,000000000000001).
Hope it's clear enough.

@guparan

This comment has been minimized.

Copy link
Member

guparan commented Oct 19, 2017

From SOFA-dev meeting:
Hi @raffaellatrivisonne, is there a chance that the argument y is rounded to 1 or -1 due to anything else than a very small numerical error? If yes, we should add an epsilon to ensure the error is acceptable.

@guparan guparan added the pr: fix label Oct 19, 2017

@untereiner

This comment has been minimized.

Copy link
Contributor

untereiner commented Oct 20, 2017

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

@guparan

This comment has been minimized.

Copy link
Member

guparan commented Oct 23, 2017

asin might be undefined, it is still better to have it crashing instead of working were it should not - again, if y contains a "big" error (greater than 10e-15 upon @raffaellatrivisonne comment).
The question is: could y contain such a "big" error? If no, let's just assume that and round this way. If yes or don't know, let's add a tiny security with something like

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;
}
@untereiner

This comment has been minimized.

Copy link
Contributor

untereiner commented Oct 23, 2017

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

@guparan

This comment has been minimized.

Copy link
Member

guparan commented Oct 23, 2017

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.

@untereiner

This comment has been minimized.

Copy link
Contributor

untereiner commented Oct 23, 2017

Well, the message sounded like "you have to add an epsilon value" 🙄 . I let @raffaellatrivisonne do her job here for now on... good luck

@raffaellatrivisonne

This comment has been minimized.

Copy link
Contributor Author

raffaellatrivisonne commented Nov 9, 2017

Hi everybody,
sorry for being late with my answer.
@guparan, as soon as the argument of asin is greater than 1, you have an invalid operation.
It doesn't matter how "big" is your error, it can be huge or infinitesimal.
In this case, it is due to numerical errors, that's why I said 10e-15.
If it is the proper way, I can set an epsilon, but then how ?
As @untereiner says, why 1e-15 instead of 1e-11 ?
How do you set a parameter on numerical errors ?

@fjourdes

This comment has been minimized.

Copy link
Contributor

fjourdes commented Nov 9, 2017

Just passing by, I do not want to raise a flame war but actuall asin method has some documentation,
espcially when it comes to domain error:
http://www.cplusplus.com/reference/cmath/asin/
Just quoting;

If a domain error occurs:

  • And math_errhandling has MATH_ERRNO set: the global variable errno is set to EDOM.
  • And math_errhandling has MATH_ERREXCEPT set: FE_INVALID is raised.

Then looking at
http://www.cplusplus.com/reference/cmath/math_errhandling/
The default behavior for math_errhandling is MATH_ERRNO, so as the doc suggest you can just check for the errno (thread-local) global variable value, and if it is set to EDOM after asin is called, then you can throw whatever error message you want.

This is probably not relevant here, since I presume the checks are there because in theory when a quaternion is normalized the value of
Real(2.)*(q[3]*q[1] - q[2]*q[0])
should always belong to the range [-1;1]. The only reason it might not be is for some numerical drifting issues (?)
Provided this assumption is correct you are indeed totally allowed to clamp the values there.

@guparan

This comment has been minimized.

Copy link
Member

guparan commented Nov 9, 2017

Thanks for the explanations @raffaellatrivisonne and @fjourdes, let's merge then ! :-)

@guparan guparan added status: ready and removed status: wip labels Nov 9, 2017

@damienmarchal

This comment has been minimized.

Copy link
Contributor

damienmarchal commented Nov 10, 2017

[ci-build]

@damienmarchal

This comment has been minimized.

Copy link
Contributor

damienmarchal commented Nov 10, 2017

Do someone knows why do tests are failing ? (In QuaterTest)

@raffaellatrivisonne

This comment has been minimized.

Copy link
Contributor Author

raffaellatrivisonne commented Nov 10, 2017

I'm taking a look.

@raffaellatrivisonne

This comment has been minimized.

Copy link
Contributor Author

raffaellatrivisonne commented Nov 10, 2017

I'm trying to debug to see where it fails, but it will take a little bit more than expected.
Meanwhile I just want to highlight that coming back to the un-fixed version (the one without my commit) the test doesn't fail YET the toEulerVector is doing an invalid operation (the bug that my commit is supposed to fix).

screenshot from 2017-11-10 12-13-39

@guparan

This comment has been minimized.

Copy link
Member

guparan commented Nov 10, 2017

So if I understood well we have 2 problems here:

  • q1 and p1 should not be nan
  • EXPECT_EQ(p0,p1); should not pass with nan values for p1
@raffaellatrivisonne

This comment has been minimized.

Copy link
Contributor Author

raffaellatrivisonne commented Nov 10, 2017

The inital problem is the vector (that in my screenshot I call "q0 euler") which has its second component which is NaN.
It comes from the unfixed toEulerVector "q0 euler"= q0.Quater::toEulerVector()
So, it doesn't surprises me that q1 and p1 are NaN, As they derive from:
Quater q1 = Quater::createQuaterFromEuler(q0.Quater::toEulerVector());

    sofa::defaulttype::Vec<3,double> p1 = q1.Quater<double>::rotate(p);

But definetively EXPECT_EQ(p0,p1) should not pass with nan values for p1

@fjourdes

This comment has been minimized.

Copy link
Contributor

fjourdes commented Nov 10, 2017

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.
So with the current implementation adding something to the EulerAngle test like

for(std::size_t i=0; i<q0.size(); ++i) // same goes for q1
{
    ASSERT_FALSE(std::is_nan(q0[i]));
}
@raffaellatrivisonne

This comment has been minimized.

Copy link
Contributor Author

raffaellatrivisonne commented Nov 23, 2017

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.
As suggested by Francois, I added the error message in Quater_test.cpp.
Now is failing, as with the old-code (without my commit) NaN values may appear.
In Quater.inl (function toEulerVector) I went back to the old code commenting the modifications I made with my commit. This way, if someone else takes the hands on this PR, he will better know what to do.

raffaellatrivisonne and others added some commits Jun 7, 2017

@guparan guparan force-pushed the mimesis-inria:Quater_fix branch from 823bd99 to 5e666a7 Sep 10, 2018

@guparan

This comment has been minimized.

Copy link
Member

guparan commented Sep 10, 2018

Hey, I just cleaned this oooooold PR.
Should be working fine 🎉

@epernod epernod merged commit 5e915d1 into sofa-framework:master Sep 12, 2018

6 checks passed

Jk2/Dashboard Builds triggered.
Details
Jk2/Scene tests Ignored. Use [ci-build][with-scene-tests] to trigger.
Details
Jk2/centos_clang-3.4_options Build OK. FIXME: 0 unit tests
Details
Jk2/mac_clang-3.5_options Build OK. FIXME: 1 unit tests
Details
Jk2/ubuntu_gcc-5.4_options Build OK. FIXME: 0 unit tests
Details
Jk2/windows7_VS-2015_options_amd64 Build OK. FIXME: 1 unit tests
Details

@untereiner untereiner deleted the mimesis-inria:Quater_fix branch Sep 12, 2018

@guparan guparan added this to the v18.12 milestone Oct 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment