Flag for relative error

[varunagrawal]

I identified an error case where the current numerical comparison fails. I have added a test and added the corresponding flag to opt for relative error which is true by default.

[gchenfc]

Random note: in line 51 of Vector.cpp there's a std::isnan even though there was already a using std::isnan in that function.

[gchenfc]

Should there also be a check that absolute==true for the absolute test case?
e.g.
a = 0.1
b = 0.2
tol = 0.5
absolute = false

should return false, but instead will return true.
I would propose line 65 to be something like
else if ( absolute && abs(a - b) <= tol )
since the case "when comparing numbers near zero" is covered by the previous test case

[gchenfc]

actually on second thought this might be easier to read if it went something like:

if absolute {
  return abs(a-b) <= tol;
} else {
  return abs(a-b) <= tol * min(larger, std::numeric_limits<double>::max());
}

[varunagrawal]

It should return true right? The equality check is more of switch-case rather than explicit absolute and relative error comparisons. The issue is that the flag is poorly named, it should be something like "check_relative" which means that we are also including a relative error check. Thanks for pointing that out (and this is why, kids, you do code-review).

[varunagrawal]

It's important to think that this is a general equality check, which is why we have a separate equal_with_abs_tol function.

[gchenfc]

Ahh gotcha!

This is nitpicky but check_relative implies to me that, when check_relative=true, it has to pass both absolute and relative checks in order to return true. i.e., it will check absolute, and if it passes, then it will also check relative. This is in contrast to the actual behavior which is that when check_relative=true, then either absolute or relative passing will pass the test and return true.

Possibly something like "allow_relative" or something? Or maybe negating: "disable_relative"??? Or maybe just clarifying the docstring in Vector.h: "...flag, if true, will consider 2 values to be equal if relative error is within tolerance even if absolute error is not. If this flag is false, false, 2 values will only be considered equal if their absolute error is within tolerance. By default ..."
Also, might as well use the @param docstring notation?

Again, sorry this is nitpicky.

[gchenfc]

Ahh gotcha!

This is nitpicky but check_relative implies to me that, when check_relative=true, it has to pass both absolute and relative checks in order to return true. i.e., it will check absolute, and if it passes, then it will also check relative. This is in contrast to the actual behavior which is that when check_relative=true, then either absolute or relative passing will pass the test and return true.

Possibly something like "allow_relative" or something? Or maybe negating: "disable_relative"??? Or maybe just clarifying the docstring in Vector.h: "...flag, if true, will consider 2 values to be equal if relative error is within tolerance even if absolute error is not. If this flag is false, false, 2 values will only be considered equal if their absolute error is within tolerance. By default ..."
Also, might as well use the @param docstring notation?

Again, sorry this is nitpicky.

[varunagrawal]

No this is great. If the flag name doesn't make sense now, no way is it going to make sense to someone 3 years from today.

I like allow_relative but I'm not sure if it describes what you said amply. Perhaps something like check_relative_error_also, but that's just crazy long (or is it?).

Guess the better thing is to update the docs.

[ProfFan]

Sorry for being late in the game, but I think require_rel is probably a good name for this?

[gchenfc]

I think it's actually the opposite of require_rel, since will always return true if the absolute error is within tolerance, but it will also return true if the absolute error is out of tolerance but the relative error is within tolerance. In other words, it is giving you the option of relative error in addition to the default behavior of absolute error. So it would be more like, !require_absolute ?
I kind of like check_relative_error_also😂 🤷‍♂️
or permit_relative_match or something? 🤷‍♂️

[varunagrawal]

Updated the flag name to check_relative_also and made updates to the docs. Running local tests before pushing. 🙂