Catch2: Approx(0.0) does not approx anything

Similar to https://github.com/catchorg/Catch2/issues/1079, https://github.com/catchorg/Catch2/issues/1096, and https://github.com/catchorg/Catch2/issues/1174.

Right now, REQUIRE(value == Approx(0.0)) will fail unless value is exactly zero. That is, Approx(0.0) has no effect and value == Approx(0.0) behaves the same as value == 0.0 (not sure about -0.0 though). The reason is that with the current implementation, the epsilon used for comparison is multiplied with zero and thus has no effect.

That is unexpected. I understand the reason why this happens, and I understand that float comparisons are hard. However, this makes using catch harder to use with no advantage obvious to the user, and I do think there has to be a better way, even if it’s just a special case for zero.

In any case, I would suggest to at least document that you cannot use Approx on zero, even though I would much prefer if the practicality and user experience would take precedence over the technical purity of the current solution.

About this issue

  • Original URL
  • State: closed
  • Created 6 years ago
  • Reactions: 4
  • Comments: 20 (7 by maintainers)

Commits related to this issue

Most upvoted comments

Just encountered this issue as well. I understand the floating point issue, however I think it is a bug in the sense that asking the API doing something does nothing. One solution is to force the user to provide a margin for Approx(0) In the current standard I don’t think it’s possible to do it at compile time, but surely possible at runtime.

I just stumbled on this problem and lost some time figuring out why the approximation didn’t work. I let you decide what’s the best solution for this but in the meantime, can this be at least explicitly documented ? This would probably help people to not fall into this trap

@tdegeus I agree, it seems broken that it does not work at all. Even if it worked by requiring the user to set a default epsilon for Approx that would be better than it is now.

NOTE: If I’m not mistaken, a user-changed non-zero margin does allow us to compare with 0.0 (Approx(1.401e-45f).margin(1.401e-45f) == 0.0) evaluates to true), so the check would need to be in the evaluation function, not contructor.

This is correct. The constructor cannot guard against 0, because Approx(0).margin(0.5) is a perfectly fine approx instance to create. Overall I am currently not sold on having the check in the comparison function itself either. The reason is that whether it is opt-in or it is opt-out, tthe warning will kinda suck.

The reason is that if it is on-by-default, there has to be a way to suppress it, so it requires adding a new CLI flag as well. It also If it is off-by-default, then it is mostly useless, as people are unlikely to get bitten by this repeatedly, which is the main reason to have a warning flag for something.

About docs, the current Approx docs haven’t been updated in ages. The two tell-tale signs are that it still uses “Catch” as the framework name, and that the markdown sources do not adhere to a column limit, unlike docs written in the last 3+ years. I am going to rewrite the docs when I find extra time, including some introduction into floating point comparisons, but that does run into the perennial problem; I have to find free time.

I guess we all agree, as the linked blog post explains, that floating point comparison is difficult and that’s why use Approx to try to deal with this complexity.

The but core issue remains, Approx(0.0) being a valid construct is a problem because the API lies to you. There’s no “approximately equal” test here, just an equality check in disguise.

The doc still doesn’t cover this issue so we can expect people falling into the same trap.

And the code doesn’t check for this particular case to warn the users. But looking at the other member functions, CATCH_ENFORCE is being used to make sure given values are within an acceptable range. Why the constructor couldn’t do something like CATCH_ENFORCE(value != 0.0, "Approx can't be used to compare with zero. link_to_the_docs_for_why") ? Sure, a fallback strategy for comparison to zero would be better but in the meantime it would make sure that users are not misusing the API.

Many developers have spent too much time on this for many moons now. There’s no perfect solution, that’s why not providing a solution is also a solution. But I believe as previously said that my suggested solution is better the the current situation.

Add strict mode that forces the user to supply a margin for Approx(0) and make it default. For backward compatibility, current implementations should just disable strict mode. Yes, technically it is a breaking change.

A more subtle approach would be to have strict mode disabled by default and provide a prominent documentation for new users.

@Danielhu229 writes:

A effective comparison between doubles should compare the exponent bits and fraction bits separately, 0.0 can be well handled in this case.

Not around zero. The problem isn’t the zero point, it’s the fact that floating-point numbers near zero (over 8 million values) are denormalized. For these values, the exponent and mantissa are mixed together. Particularly around zero, there’s a danger of advertising one approach as the right one. In this particular case, where you’re testing around a fixed value, an epsilon value can make sense (though not a thoughtlessly chosen one). An ULP comparison ends up being equivalent, again, due to the fixed comparison point. However, there’s a huge relative leap from the smallest positive value to zero to the largest negative value. In addition, a sign-change here could mean a clear error that should be flagged.

For all these reasons, I’d prefer the following example code, as it’s much simpler, vastly more clear, and does not depend on the reader having memorized the underlying current implementation of Catch2’s Approx() helper:

REQUIRE (x > 0)
REQUIRE (x < 1e-140)