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

Double comparison corrections #151

Merged
merged 2 commits into from
Nov 3, 2017

Conversation

d-meiser
Copy link
Contributor

@d-meiser d-meiser commented Nov 1, 2017

This fixes #148 and fixes #149. It does not address #147.

In my opinion, given the way double comparison currently works in cgreen, #147 is expected behavior. significant_figures always indicates a relative precision, never an absolute tolerance. Perhaps #147 should be a feature request for an absolute tolerance. But this would be in addition to significant_figures. One way to add this capability is by introducing something like doubles_are_close(double tried, double expected, double epsilon). This is what gtest and the boost unit test framework do. Or there could be a absolute_tolerance_for_assert_double_is function which sets a cgreen static variable analogous to significant_figures_for_assert_double_are.

The absolute tolerance introduced in this patch leads to a (cosmetic?) issue. An assertion like assert_that_double(x, is_not_equal_to_double(0)) can fail because x was within the absolute tolerance of 0. But our error message suggests that the assertion fails because x is within the relative epsilon of 0. I don't know of a good way to fix this. If we think this is important enough I can create an issue with a test case and more in-depth discussion.

/* Issue #147

DM: I don't think this test case is valid. We should delete it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is essentially rehashing the discussion from #147. I propose that we remove this test case as it is expected behavior given the way significant_figures works. I can fix up the pull request depending on our decision.

@coveralls
Copy link

coveralls commented Nov 1, 2017

Coverage Status

Coverage decreased (-0.09%) to 96.097% when pulling a12f40b on d-meiser:double_comparison_corrections into 2c146fa on cgreen-devs:master.

@thoni56
Copy link
Contributor

thoni56 commented Nov 1, 2017

I think this looks good, and, without any deeper pondering, agree to the invalidation of #147. I'll relabel that as a feature request.

Could you make an attempt at a description of this in the manual, please? Then we can point to RTM if #147 comes up again. (And when we forget...)

@d-meiser
Copy link
Contributor Author

d-meiser commented Nov 1, 2017

Ok, sounds good. I'll write up something for the manual. Will add it to this pull request. Thanks!

@d-meiser d-meiser force-pushed the double_comparison_corrections branch from a12f40b to 5b40aa6 Compare November 2, 2017 02:56
@coveralls
Copy link

coveralls commented Nov 2, 2017

Coverage Status

Coverage increased (+0.01%) to 96.201% when pulling 5b40aa6 on d-meiser:double_comparison_corrections into 2c146fa on cgreen-devs:master.

@d-meiser
Copy link
Contributor Author

d-meiser commented Nov 2, 2017

I've changed the pull request as follows:

Let me know if further changes are needed. Thanks!

@thoni56
Copy link
Contributor

thoni56 commented Nov 2, 2017

Excellent text explaining everything!

Maybe we need a cross-reference to that text from the section Working with doubles? Or it might even be moved over there and a reference in the other direction?

If I wanted to read up on how to use doubles, I think I would look up that section first. So it's a matter of how much details should go there.

@d-meiser
Copy link
Contributor Author

d-meiser commented Nov 2, 2017

That's a good suggestion. I'll move it to the Working with doubles section and cross reference. Thanks!

* Introduce an absolute tolerance.
* Improve implementation of accuracy function to avoid casting and make the
  logic more transparent.
* Add discussion of floating point comparisons to manual.
* Add a test case.
* Minor cosmetic changes (e.g. renamed a test case).
* Remove a test case that compares a small number with 0 because this is
  expected behavior.

Discussion

The introduction of an absolute tolerance fixes the underflow issues near 0. If
two numbers are within the absolute tolerance of one another they are considered
equal and we don't need to evaluate the problematic accuracy function. We choose
an absolute tolerance that is 10^8 times larger than the smallest double number
that can be represented. Eight is the default number of significant digits used
for relative comparisons. This prevents underflow issues when comparing numbers
near zero for the default number of significant digits.

Note that our default cannot prevent underflow if the user increases the number
of significant digits to a value larger than eight. doubles_are_equal will not
crash in this case but it may wrongly report two numbers as equal. The
problematic cases are numbers near zero.

We have added a discussion of our floating point comparison algorithm to the
manual. We point out the problems that can occur when comparing numbers close to
zero and we propose to use constraints of the form

assert_that_double(fabs(x - y), is_less_than_double(abs_tolerance));

in that case.
@d-meiser d-meiser force-pushed the double_comparison_corrections branch from 5b40aa6 to 050c936 Compare November 3, 2017 02:10
@coveralls
Copy link

coveralls commented Nov 3, 2017

Coverage Status

Coverage increased (+0.01%) to 96.201% when pulling 050c936 on d-meiser:double_comparison_corrections into 2c146fa on cgreen-devs:master.

@d-meiser
Copy link
Contributor Author

d-meiser commented Nov 3, 2017

I've updated the documentation:

  • Move discussion of floating point comparison to section Working with doubles
  • Add a cross reference to the section on how to write constraints for mocks involving doubles
  • Minor tweaks to working and type setting.

Let me know if this needs further changes. Thanks!

@thoni56 thoni56 merged commit a9dc60d into cgreen-devs:master Nov 3, 2017
@thoni56
Copy link
Contributor

thoni56 commented Nov 3, 2017

Thanks @d-meiser!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants