Catch2: Approx(0.0) does not approx anything

[karyon]

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.

[horenmar]

This is a fundamental problem with 0, in that, a relative comparison with sane (< 1) epsilon will always fail when the target is 0, even if it takes into account either of the sides.

Consider expression actual == Approx(target). This evaluates to true, if | actual - target | < margin holds, so the question boils down to what is the margin like. There are 2 potential sources of margin in Approx, an absolute value set via call to setMargin, which defaults to 0, and a relative value that is currently computed as ε * | target |. However, lets consider instead consider ε * max( |target| , |actual| ) in this example.

By setting target to 0 and simplifying the above expressions, we get | actual | < ε * | actual |... For this to evaluate to true, ε > 1 has to hold, which breaks any semblance of sanity in comparisons for other cases (because it would also mean that e.g. 10 == Approx(20)).

[karyon]

I think I do understand the math and the fundamental problem. I still think that a better solution can be found, not in terms of mathematical "correctness", but in terms of matching what the user expects, which is that Approx(0.0) does actually approximate something.

I am aware that this has probably discussed to death. I hope to add another point of view in the following, if I'm just repeating what has already been said then I'm sorry.

I would suggest to either add a special case for zero, using some reasonably guesstimated fixed epsilon in that case, simply to create a practical solution for an annoyance in a common case (comparing to zero). A more elaborate rationale would be that when comparing to Approx(<very small number>), chances are that the "scale" of the calculation was indeed very small (i.e. using only very small numbers), so using an even smaller epsilon is fine. However, when comparing to Approx(0.0), chances are that the calculation was actually done at a non-zero scale, since most calculations are not just about zeroes (right? :) ), and applying an epsilon of zero does not help in that case.

Alternatively I would suggest to add a minimum epsilon to the calculated one to not let the applied epsilon become ridiculously small for small numbers and ultimately zero, or in other words, to use min(ε * max( |target| , |actual| ), minimum_ε) or perhaps ε * max( |target| , |actual| ) + minimum_ε (which would be equivalent of setting the existing scale variable to some minimum_ε i think?). again, the rationale could be that the calculations are always happening at some scale that is not zero.

[horenmar]

The problem with setting a minimum ε is that it also leads to unintuitive, and what is worse, incorrect, results. Let's say that we set minimum ε to std::numeric_limits<double>::epsilon, which is the difference between 1 and the next smallest representable value, or roughly 2.2e-16. This sounds good, until you realise that while this seems insanely small, there are still 4372995238176751615 (4*10^18) numbers between 0 and std::numeric_limits<double>::epsilon...

Also, Approx actually used to have scale default to 1.0, but that lead to false positives in code such as REQUIRE(1e-14 == Approx(1e-15).epsilon(0.01)), which is obviously incorrect (1e-14 is not within 1% of 1e-15), but would pass because the scale would blow out the actual numbers used in the comparison. That and some other similar problems made me ultimately decide to go for the mathematically more correct comparison, even though it is possibly surprising. Fundamentally, comparing floats is hard, and I think it is better to surprise people who don't know what they are doing than it is to surprise people who do know what they are doing.

[karyon]

Alright, I agree that the minimum epsilon is a very inconsistent solution. Any practical downside for the special-case-for-zero-solution? I can't imagine people who know what they are doing to expect Approx(0.0) to not approximate anything or to be surprised if it did approximate something.

[jayeshbadwaik]

@karyon Would you be happy with using ULP matchers instead? I think that is the better way to achieve what you want.

[karyon]

Thanks. However, I can't explain my colleagues that they can use Approx if they want, but for zeroes, that does not work and they should use something else instead. We preferred practicality and modified Approx instead (we set the scale to std::numeric_limits::epsilon by default i think)

[Danielmelody]

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

[hollasch]

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

[TheNicker]

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.

[BenjaminNavarro]

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

[dimaga]

Previously, Approx::m_scale was set 1.0 by default and there were no problems with Approx(0.0). By the recent version, Approx::m_scale default value has been changed to 0.0. Does anybody know, why?

[hollasch]

I think that as @jayeshbadwaik commented above, ULP comparison is the way to go. std::frexp() provides an easy way to accomplish this. It makes much more sense to me to assert that f1 and f2 agree to _n_ bits.

[tdegeus]

I can only very highly endorse @BenjaminNavarro remark of at least documenting this. But in the end I also think that the API should allow for

REQUIRE(a == Approx(0.0))

to work...

[tjwrona1992]

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