librascal icon indicating copy to clipboard operation
librascal copied to clipboard

constexpr static TOLERANCE in files to be used from rascal::math for unification

Open mastricker opened this issue 5 years ago • 4 comments

In the half and full neighbourlist adaptor this constexpr variable pops up. I'm gonna redirect that to the math definitions to be consistent.

mastricker avatar Nov 10 '19 18:11 mastricker

I am not sure we want to unify math::DBL_FTOL and the tests TOLERANCE.

One is for internal use inside librascal implementation, the others are test-specific tolerances. Different tests already use different tolerances, from 1e-10 to 1e-14.

math::DBL_TOLERANCE is defined as 100.0 * std::numeric_limits<double>::epsilon(), which is 100 * 2.22045e-16, so roughly 2e-14.

I am also not convinced by the need for a single tolerance value for all of rascal functions, and why it is set to this specific value. std::numeric_limits<double>::epsilon() is defined as the distance between 1.0 and the next representable double, and thus refer to the rough precision of double around 1.0. But this precision is not the same across the whole range of double values.


I think we should in general use relative tolerances (abs((value - reference) / reference) < TOL) instead of absolute tolerances (abs(value - reference) < TOL), since they make more sense and are uniform around the range of double values. We might also want to look into ULPs (units of least precision), since they are designed for floating point comparison.

See these very nice articles for more information on floating point values comparison: https://bitbashing.io/comparing-floats.html, https://randomascii.wordpress.com/2012/02/25/comparing-floating-point-numbers-2012-edition/

Luthaf avatar Nov 10 '19 19:11 Luthaf

Well, you make a good case, can't argue with that.

Then I'll reformulate that issue to change double comparisons to relative double comparisons and keep the local tolerance?

On Nov 10, 2019 20:37, Guillaume Fraux [email protected] wrote:

I am not sure we want to unify math::DBL_FTOL and the tests TOLERANCE.

One is for internal use inside librascal implementation, the others are test-specific tolerances. Different tests already use different tolerances, from 1e-10 to 1e-14.

math::DBL_TOLERANCE is defined as 100.0 * std::numeric_limits::epsilon(), which is 100 * 2.22045e-16, so roughly 2e-14.

I am also not convinced by the need for a single tolerance value for all of rascal functions, and why it is set to this specific value. std::numeric_limits::epsilon() is defined as the distance between 1.0 and the next representable double, and thus refer to the rough precision of double around 1.0. But this precision is not the same across the whole range of double values.


I think we should in general use relative tolerances (abs((value - reference) / reference) < TOL) instead of absolute tolerances (abs(value - reference) < TOL), since they make more sense and are uniform around the range of double values. We might also want to look into ULPs (units of least precision), since they are designed for floating point comparison.

See these very nice articles for more information on floating point values comparison: https://bitbashing.io/comparing-floats.html, https://randomascii.wordpress.com/2012/02/25/comparing-floating-point-numbers-2012-edition/

You are receiving this because you were assigned. Reply to this email directly, view it on GitHubhttps://github.com/cosmo-epfl/librascal/issues/210?email_source=notifications&email_token=AA44BXXRQIDK5U7UMQWIVM3QTBPC5A5CNFSM4JLNRZC2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEDVE24A#issuecomment-552226160, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AA44BXW44HZ4PSSDSASLGO3QTBPC5ANCNFSM4JLNRZCQ.

mastricker avatar Nov 10 '19 19:11 mastricker

Was this addressed in a recent PR (#196 perhaps) and, if so, can we close this?

max-veit avatar Nov 18 '19 18:11 max-veit

No, this one is referring to the tests as far as I remember, Guillaume is right with his comment.

mastricker avatar Nov 19 '19 08:11 mastricker