Skip to content
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

Merged
merged 6 commits into from
Nov 19, 2020
Merged

Flag for relative error #580

merged 6 commits into from
Nov 19, 2020

Conversation

varunagrawal
Copy link
Collaborator

@varunagrawal varunagrawal commented Nov 4, 2020

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.

@varunagrawal varunagrawal added bugfix Fixes an issue or bug quick-review Quick and easy PR to review high-priority Need this done quickly labels Nov 4, 2020
@varunagrawal varunagrawal self-assigned this Nov 4, 2020
Copy link
Member

@gchenfc gchenfc left a 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;
Copy link
Member

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

Copy link
Member

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());
}

Copy link
Collaborator Author

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).

Copy link
Collaborator Author

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.

Copy link
Member

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.

Copy link
Member

@gchenfc gchenfc Nov 19, 2020

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.

Copy link
Collaborator Author

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.

Copy link
Collaborator

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?

Copy link
Member

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? 🤷‍♂️

Copy link
Collaborator Author

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. 🙂

@varunagrawal varunagrawal changed the title Flag for absolute error Flag for relative error Nov 18, 2020
@varunagrawal varunagrawal merged commit 70b04af into develop Nov 19, 2020
@varunagrawal varunagrawal deleted the fix/numerical-error branch November 19, 2020 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Fixes an issue or bug high-priority Need this done quickly quick-review Quick and easy PR to review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants