Approx is not symmetric

[paulfloyd]

Describe the bug
For certain values, Approx does not exhibit symmetric behaviour. That is, it is possible that Approx(a) == b but Approx(b) != a.

This can occur when b ~= a(1 + epsilon)

Expected behavior
The same result should be produced irrespective of whether 2 values are on the left or right.

Reproduction steps
#define CATCH_CONFIG_MAIN
#include "catch.hpp"

TEST_CASE( "Approx is symmetric", "[approx]" )
{
Approx target1 = Approx(1.0).epsilon(0.1);
Approx target2 = Approx(1.105).epsilon(0.1);
REQUIRE((target1 == 1.105) == (target2 == 1.0));
}

Platform information:

• OS: Linux
• Compiler+version: GCC v6.2.0
• Catch version: v2.9.2

Additional context
The following change should fix this issue (using std::max for the two values).

bool Approx::equalityComparisonImpl(const double other) const {
// First try with fixed margin, then compute margin based on epsilon, scale and Approx's value
// Thanks to Richard Harris for his help refining the scaled margin value
return marginComparison(m_value, other, m_margin) || marginComparison(m_value, other, m_epsilon * (m_scale + std::max(std::fabs(other), std::fabs(m_value))));
}

[horenmar]

Yes, it is not symmetric and currently that is the intended behaviour.
The idea behind it is that the argument you construct Approx with is
a "target" that you want to hit. The various knobs on it (margin,
epsilon, scale) then change how wide the target is, but wideness
of the target does not change based on the value the Approx instance
is compared to.

This is expected to be less surprising to people with only a fuzzy
understanding of comparing floating point values. Consider
Approx(10).epsilon(0.1). This says that you are aiming for 10,
give or take 10%. In other words, the interval [9, 11] (luckily
floats/doubles can represent both of those exactly, so the set is
crisp). This interval notably does not include 11.1, but if you apply
the epsilon to bigger of the two numbers, then 10 and 11.1 are
within 10% of each other.

If you want something that is scale and order independent, you can
use the WithinULP matcher.

[paulfloyd]

This is disappointing. I'm not sure what purpose you were trying to achieve by trying to explain with an example that is essentially the same as the one that I gave, except that it is scaled up by a factor of 10.

What do you consider will be user's expectations when the arguments to Approx are variables rather than literals?

WithinULP looks useful but it is no replacement for Approx

• lacks the combination of relative and absolute tolerances that Approx offers.

• it is simply not possible to use with a reasonable reltol like 1e-4 - double precision ULP is about 2.2e-16, but almostEqualUlps takes an int maxUlpDiff, so the largest possible reltol is something like 9.5e-7

see abstol reltol and vntol here

[horenmar]

I would still expect the user's expectation to be "is a close enough to b", rather than "is either a close enough to b, or b close enough to a"?

Anyway,

1. is simple to solve, that is what the WithinAbs matcher is for.
2. is a good point. When originally designed, the ULP matcher was aimed at people who need to check that their computation did not diverge from the correct one, not for significant relative differences where ULPs could be within millions. I can fix the range issue simply enough, but I'll give some more thought to providing a relative difference matcher.