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.

Sign up for GitHub

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

Jump to bottom

Flag for relative error #580

Merged
merged 6 commits into from
Nov 19, 2020
Merged

Flag for relative error #580

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
51b6fd0
Added flag for absolute error
varunagrawal Nov 3, 2020
c68a647
add test
varunagrawal Nov 3, 2020
326957b
cleaner assertion
varunagrawal Nov 4, 2020
1fd0e57
Better fkag naming, and more docs
varunagrawal Nov 18, 2020
50643be
Merge branch 'develop' into fix/numerical-error
varunagrawal Nov 18, 2020
0737a45
better flag name and docs
varunagrawal Nov 19, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion gtsam/base/Matrix.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ bool equal_with_abs_tol(const Eigen::DenseBase<MATRIX>& A, const Eigen::DenseBas

for(size_t i=0; i<m1; i++)
for(size_t j=0; j<n1; j++) {
if(!fpEqual(A(i,j), B(i,j), tol)) {
if(!fpEqual(A(i,j), B(i,j), tol, false)) {
return false;
}
}
Expand Down
16 changes: 9 additions & 7 deletions gtsam/base/Vector.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ namespace gtsam {
* 1. https://randomascii.wordpress.com/2012/02/25/comparing-floating-point-numbers-2012-edition/
* 2. https://floating-point-gui.de/errors/comparison/
* ************************************************************************* */
bool fpEqual(double a, double b, double tol) {
bool fpEqual(double a, double b, double tol, bool check_relative_also) {
using std::abs;
using std::isnan;
using std::isinf;
Expand All @@ -48,7 +48,7 @@ bool fpEqual(double a, double b, double tol) {
double larger = (abs(b) > abs(a)) ? abs(b) : abs(a);

// handle NaNs
if(std::isnan(a) || isnan(b)) {
if(isnan(a) || isnan(b)) {
return isnan(a) && isnan(b);
}
// handle inf
Expand All @@ -60,13 +60,15 @@ bool fpEqual(double a, double b, double tol) {
else if(a == 0 || b == 0 || (abs(a) + abs(b)) < DOUBLE_MIN_NORMAL) {
return abs(a-b) <= tol * DOUBLE_MIN_NORMAL;
}
// Check if the numbers are really close
// Needed when comparing numbers near zero or tol is in vicinity
else if(abs(a-b) <= tol) {
// Check if the numbers are really close.
// Needed when comparing numbers near zero or tol is in vicinity.
else if (abs(a - b) <= 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 •
edited
Loading

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

}
// Use relative error
else if(abs(a-b) <= tol * min(larger, std::numeric_limits<double>::max())) {
// Check for relative error
else if (abs(a - b) <=
tol * min(larger, std::numeric_limits<double>::max()) &&
check_relative_also) {
return true;
}

Expand Down
8 changes: 7 additions & 1 deletion gtsam/base/Vector.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,9 +85,15 @@ static_assert(
* respectively for the comparison to be true.
* If one is NaN/Inf and the other is not, returns false.
*
* @param check_relative_also is a flag which toggles additional checking for
* relative error. This means that if either the absolute error or the relative
* error is within the tolerance, the result will be true.
* By default, the flag is true.
*
* Return true if two numbers are close wrt tol.
*/
GTSAM_EXPORT bool fpEqual(double a, double b, double tol);
GTSAM_EXPORT bool fpEqual(double a, double b, double tol,
bool check_relative_also = true);

/**
* print without optional string, must specify cout yourself
Expand Down
13 changes: 13 additions & 0 deletions gtsam/base/tests/testMatrix.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1163,6 +1163,19 @@ TEST(Matrix , IsVectorSpace) {
BOOST_CONCEPT_ASSERT((IsVectorSpace<Vector5>));
}

TEST(Matrix, AbsoluteError) {
double a = 2000, b = 1997, tol = 1e-1;
bool isEqual;

// Test only absolute error
isEqual = fpEqual(a, b, tol, false);
EXPECT(!isEqual);

// Test relative error as well
isEqual = fpEqual(a, b, tol);
EXPECT(isEqual);
}

/* ************************************************************************* */
int main() {
TestResult tr;
Expand Down
6 changes: 3 additions & 3 deletions gtsam/geometry/tests/testRot3.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -807,15 +807,15 @@ TEST(Rot3, RQ_derivative) {
test_xyz.push_back(VecAndErr{{0, 0, 0}, error});
test_xyz.push_back(VecAndErr{{0, 0.5, -0.5}, error});
test_xyz.push_back(VecAndErr{{0.3, 0, 0.2}, error});
test_xyz.push_back(VecAndErr{{-0.6, 1.3, 0}, error});
test_xyz.push_back(VecAndErr{{-0.6, 1.3, 0}, 1e-8});
test_xyz.push_back(VecAndErr{{1.0, 0.7, 0.8}, error});
test_xyz.push_back(VecAndErr{{3.0, 0.7, -0.6}, error});
test_xyz.push_back(VecAndErr{{M_PI / 2, 0, 0}, error});
test_xyz.push_back(VecAndErr{{0, 0, M_PI / 2}, error});

// Test close to singularity
test_xyz.push_back(VecAndErr{{0, M_PI / 2 - 1e-1, 0}, 1e-8});
test_xyz.push_back(VecAndErr{{0, 3 * M_PI / 2 + 1e-1, 0}, 1e-8});
test_xyz.push_back(VecAndErr{{0, M_PI / 2 - 1e-1, 0}, 1e-7});
test_xyz.push_back(VecAndErr{{0, 3 * M_PI / 2 + 1e-1, 0}, 1e-7});
test_xyz.push_back(VecAndErr{{0, M_PI / 2 - 1.1e-2, 0}, 1e-4});
test_xyz.push_back(VecAndErr{{0, 3 * M_PI / 2 + 1.1e-2, 0}, 1e-4});

Expand Down