-
Notifications
You must be signed in to change notification settings - Fork 767
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
Flag for relative error #580
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -66,7 +66,9 @@ bool fpEqual(double a, double b, double tol) { | |||
return true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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());
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's important to think that this is a general equality check, which is why we have a separate equal_with_abs_tol
function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for being late in the game, but I think require_rel
is probably a good name for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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? 🤷♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the flag name to check_relative_also
and made updates to the docs. Running local tests before pushing. 🙂
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.