Kratos: [Core] Tolerances

[GuillermoCasas]

I think it is not good to have such varied range of tolerances being used with KRATOS_CHECK_NEAR(a,b, tolerance). In my opinion, it would be better practice to use a common tolerance everywhere for most cases and only set the tolerance for those cases where round-up errors are an issue (related to comments made by @msandre in #4246).

I therefore propose to both @KratosMultiphysics/design-committee and @KratosMultiphysics/implementation-committee to consider the introduction of KRATOS_STANDARD_CHECK_NEAR(a,b) which implements a check based on a base Kratos tolerance, perhaps one based on 'std::numeric_limits::epsilon()'. The macro could use the value of the second argument to scale the epsilon (and perhaps some extra security factor).

This would have the additional advantage of correcting for scaling automatically, since it is not that common of developers to make sure the variables they are comparing to a numerical tolerance are appropriately made dimensionless.

[loumalouomega]

I don't agree with this. Some calculation can not be precise enough to use epsilon as tolerance. I prefer to have a default argument with epsilon, but keeping the possibility of using a custom tolerance.

[pablobecker]

I agree with Vicente. each method as an expected different accuracy and hence the need to control it. epsilon is simply too small for most cases.

[GuillermoCasas]

As mentioned in my comment and discussed during the @KratosMultiphysics/implementation-committee meeting, the tolerance does not need to be epsilon itself, but proportional to it. In fact, of the form

where C would be some large-enough number.
Note the asymmetry between first and second arguments. This is essential if one wants to have a robust dimension-blind comparison (which argument provides the correct scaling otherwise?). If one wants a symmetric function, then one has to do the normalization outside the function.

In the latter case, I would be in favour of having a fixed numeric tolerance in the form of assertEqual of python. Then if people are using it with very small or very large quantities it is their own fault.

Let us keep this discussion going

[GuillermoCasas]

Actually, I made a mistake: tol(a,b) = C eps (1 + |a| + |b|) should work and it is symmetric.

[msandre]

tol(a,b) = C eps (1 + |a| + |b|)

To me it looks reasonable. I think one should choose C eps to be significantly larger than std::numeric_limits<double>::epsilon(). Another formulation would be C eps (C eps + |a| + |b|) with C eps >> sqrt(std::numeric_limits<double>::epsilon())`. This would a more "relaxed" tolerance which may cause fewer exceptions where one needs to intervene.

[GuillermoCasas]

See PR #4580.

[RiccardoRossi]

i thought i commented this, but apparently i did not .

this check is ok if your values are in the order of 1 ... it is not if you are in the order of zero

[pooyan-dadvand]

If the values are close to zero then it would work as std::numeric::epsilon which won't solve the problem you have

[AlejandroCornejo]

What if we consider a tolerance calculation of this kind:
tol = max(AbsTol, RelTol * abs(max(a,b)))
In this way we could take into account the relative and absolutes differences in some way. The problematic is to define those Abs and Rel tolerances...

[RiccardoRossi]

that s an interesting idea.

i need to think about this

On Sun, May 12, 2019, 12:03 PM Alejandro Cornejo Velázquez <
[email protected]> wrote:

What if we consider a tolerance calculation of this kind:
tol = max(AbsTol, RelTol * abs(max(a,b)))
In thi way we could take into account the relative and absolutes
differences in some way. The problematic is to define those Abs and Rel
tolerances...

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/KratosMultiphysics/Kratos/issues/4305#issuecomment-491581943,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AB5PWEILHYGIYMEE3UTSTT3PU7TOXANCNFSM4G3C5N7Q
.

[GuillermoCasas]

What if we consider a tolerance calculation of this kind:
tol = max(AbsTol, RelTol * abs(max(a,b)))
In this way we could take into account the relative and absolutes differences in some way. The problematic is to define those Abs and Rel tolerances...

What you propose is equivalent to my proposal in order-of-magnitude terms. Let us assume AbsTol = RelTol for the sake of the argument. I am also going to assume you meant max(abs(a),abs(b)), since otherwise the behaviour would be different for negative and positive quantities. Then we obtain
tol = AbsTol * max(1, max(|a|,|b|)), which is of the same order of magnitude as AbsTol * (1 + |a| + |b|) both for very small or very large a and b values.

[AlejandroCornejo]

The @KratosMultiphysics/implementation-committee has reached consensus with the following reasons:

1. The most common complain was that it was difficult to detect differences between very small numbers (compare 1e-12 and 1.01e-12 for example). However it is not clear that these 2 numbers are in fact different... This is only true if the characteristic scale of the problem were of order 1e-12, say. In a different context (i.e. characteristic scale of order 1) they would be essencially equal. Therefore this information cannot be extracted from a and b alone... Our suggestion is to assume by defaul that the referemce scale is not too far from order 1.

2. In practice, it is observed that researchers use very rough heuristics to determine the size of the tolerances they pass to the macro (i.e., 1e-6). It is for these researchers that the standard macro would be the most useful.

In summary, this standard macro is complementary to the KRATOS_CHECK_NEAR in which the tolerance can be set by the user.

PS: @adityaghantasala once you gice the OK we will put the consensus label

[RiccardoRossi]

here goes my personal comment

As I was telling before and to some of you separately, i agree that the proposed way of comparing numbers is better than others.

I do believe however that instead of having this as default we should oblige users to specify the order of magnitude of they results they want to compare.
If one really wants, he can specify "1" and live with that, however i believe it is a minimal effort to specify the order correctly.

aside of this, we shall AT LEAST OPTIONALLY allow to specify one tolerance other than the one that is used by default.

[GuillermoCasas]

here goes my personal comment

As I was telling before and to some of you separately, i agree that the proposed way of comparing numbers is better than others.

I do believe however that instead of having this as default we should oblige users to specify the order of magnitude of they results they want to compare.
If one really wants, he can specify "1" and live with that, however i believe it is a minimal effort to specify the order correctly.

aside of this, we shall AT LEAST OPTIONALLY allow to specify one tolerance other than the one that is used by default.

Riccardo, the option is there by calling the already working macro. That is, KRATOS_CHECK_NEAR will keep working as it does now, with a tolerance that is imposed. I am suggesting adding an additional macro, KRATOS_STANDARD_CHECK_NEAR , that does not require the introduction of that tolerance.

[AlejandroCornejo]

This issue has become stale. the @KratosMultiphysics/implementation-committee thinks that the course of action should be one of the following two options:

1. Close the issue
2. Accept the basic content of #4580.

We request the @KratosMultiphysics/technical-committee to pic one of the above. If in two weeks there's no answer we just go with option 1.

[AlejandroCornejo]

ping @KratosMultiphysics/technical-committee

[adityaghantasala]

ping @KratosMultiphysics/technical-committee

[RiccardoRossi]

i don t think that it should be accepted as such...but what i am.asking is simply to add a number with the order of magnitude of the numbers to compare to. that can coexist easily with the existing macros and would be a valuable addition to the current capabilities...so i don t see either why we should close it

[pooyan-dadvand]

Giving it another read, I see it differently now:

1. I understand that this macro is for tests and is normal that each test has its own way to determine what is close enough
2. When we are using this macro we usually know the order of magnitude of two values comparing so we can adapt our tolerance
3. The check should be macro for line information but the tolerance not.

What about having a Tolerance class which implements different tolerance mechanism as static methods?

[GuillermoCasas]

Replying to @pooyan-dadvand 's comments:

1. Not only for tests. In fact, the goal is to have a standard comparison for everyone to use and useful in a large variety of situations, excluding exceptional ones where the characteristic orders of magnitude are very small.

2. One cannot use the order of magnitude of the values presented to make the tolerance smaller (you can make it bigger, that is not a problem). This is because you could be comparing 1.0e-12 to 1.1e-12 (10% apart) in a context with large numbers. It would be unreasonable to conclude that they are far apart because of that 10% difference. They would be essentially both negligibly close in that context.

3. I am not sure I understand: do you mean that setting the tolerance as a global variable is a bad idea? This is not essential.

About the idea of making the Tolerance a class, I am ok with it, as it then would allow interesting derivations for special cases. I will discuss it within the @KratosMultiphysics/implementation-committee.

[GuillermoCasas]

After a second round of discussions, the @KratosMultiphysics/implementation-committee concluded that the best option is to go with the suggestion of @RiccardoRossi and define the stadard check as a scaled check, where three inputs have to be given: (a, b, scale), where the scale is the characteristic size of the magnitude being measured. This will make the macro a bit more involved to use but more robust.

[pooyan-dadvand]

1. Not only for tests. In fact, the goal is to have a standard comparison for everyone to use and useful in a large variety of situations, excluding exceptional ones where the characteristic orders of magnitude are very small.

I see.

2. One cannot use the order of magnitude of the values presented to make the tolerance **smaller** (you can make it bigger, that is not a problem). This is because you could be comparing 1.0e-12 to 1.1e-12 (10% apart) in a context with large numbers. It would be unreasonable to conclude that they are far apart because of that 10% difference. They would be essentially both negligibly close in that context.

I agree and I like the scaled check giving the scale to be considered.

3. I am not sure I understand: do you mean that setting the tolerance as a global variable is a bad idea? This is not essential.

As @RiccardoRossi mentioned if we separate the tolerance and error handling, we can have a function for the tolerance one and then a macro for sending the exception. The macro is needed to fill the code location for the exception but not for tolerance.

About the idea of making the Tolerance a class, I am ok with it, as it then would allow interesting derivations for special cases. I will discuss it within the @KratosMultiphysics/implementation-committee.

This is something parallel which can coexist with the current decision.

[GuillermoCasas]

Great, I will make the modifications soon to #4580 according to what has been agreed. About your explanation, @pooyan-dadvand , I understand now. Yes, I guess it would make sense then to have a class provide the error which is fed to the macro that does error handling. I think the first step though is to implement the macro as proposed and then give some more thought to it and some time to test it before coming up with a nice design. At the moment, I already see the advantage of having a class that can have default tolerances for the standard cases that do not need rescaling etc.

[GuillermoCasas]

I have not yet implemented these changes because, in fact, I now think they are pointless. My initial intention was to bypass completely the need for an extra input (a tolerance or whatever) to the common user. By adding a scaling factor, we are actually making the user unsure of what that number should be, forcing him to look at the code in detail to find out why. If he is doing all that, then he should also be able to come up with an appropriate tolerance, for which the KRATOS_CHECK_NEAR macro is already provided.

Thus, I now think that adding the second version of the macro makes no sense, and given the criticism to the first proposal of KRATOS_STANDARD_CHECK_NEAR, I think it is best to simply close this.

[philbucher]

maybe we could use the Approx fct that the testing framework uses?

Maybe better to just use this rather than reinvent the wheel

(I am writing this while not being up-to-date with this thread)

[GuillermoCasas]

@philbucher Yes, that seems to be in the same spirit (100 * epsilon is being used by default). I will close this after the next @KratosMultiphysics/implementation-committee meeting.

[pablobecker]

on behalf on the @KratosMultiphysics/implementation-committee , we agree with @GuillermoCasas . If the supposedly simplied CHECK_NEAR requires as many parameters as the original one, not only it does not simplifly the usage of it, but actually makes it more confusing since it becomes unclear how to set the parameters. Thus defeating the purpose of this PR.

[pooyan-dadvand]

So shall we close this? Meanwhile, I like the approximates function.