Approx is incorrect for small margins

[apGribble]

The following test will incorrectly fail:
REQUIRE(Approx(1.0).margin(0.0000001) != 1.000001);
This is because of the default value for epsilon in the Approx class and the fact that Approx::equalityComparisonImpl will pass if the value is in the range of the margin OR the epsilon values.
Changing the default value of epsilon to zero fixes the bug but doesn't provide the desired default behaviour for Assert.

[apGribble]

Replacing the following:

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::fabs(m_value)));
    }

With:

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
        if(m_margin == 0.0) return marginComparison(m_value, other, m_epsilon * (m_scale + std::fabs(m_value)));
        return marginComparison(m_value, other, m_margin);
    }

Sorts the problem for me but has slightly odd behaviour is that Approx().margin(0.0) will perform the check against epsilon rather than ensuring the inputs are identical.

[hbina]

I think the problem occurs because epsilon and margin calculation uses the same function, so both checks are executed, even though these two are different things.

Particularly, the problem arises when the accuracy of the margin is smaller than the default epsilon and so the epsilon_check will pass the test.

Perhaps a fix would be to introduce a state machine? However, I am not sure if something like
Approx(1.0).margin(0.0000001).epsilon(0.01) is used?

[horenmar]

So, I don't think this is a bug.

The reason for this is that it is very hard (well, impossible), to determine the user's intent, thus it is better to assume that the user has set up both checks correctly -- after all, if the user does not want a relative comparison, they can always set the epsilon to zero. In fact, I think that using both tolerances and taking the most forgiving one is the superior option to implement things like the Approx Matcher (#1499).

[apGribble]

I think the current behaviour is very misleading as you wouldn't expect Approx().margin(n) to check against both a margin and epsilon.

For me hbina's solution is much more logical. If you add another state then I think it deals with cases where you want to use margin, epsilion or both.
E.g.

switch (current_calculation_type) {
            case BOTH: {
                is_equal = marginComparison(m_value, other, m_margin) ||
                           marginComparison(m_value, other, m_epsilon * (m_scale + std::fabs(m_value)));
                break;
            }
            case MARGIN: {
                is_equal = marginComparison(m_value, other, m_margin);
                break;
            }
            case EPSILON:
            case DEFAULT: {
                is_equal = marginComparison(m_value, other, m_epsilon * (m_scale + std::fabs(m_value)));
                break;
            }
            default: {
                // How is this possible???
            }
        }
        return is_equal;
    }
    void Approx::setMargin(double margin) {
        CATCH_ENFORCE(margin >= 0,
            "Invalid Approx::margin: " << margin << '.'
            << " Approx::Margin has to be non-negative.");
        m_margin = margin;
        if( current_calculation_type == EPSILON ) {
            current_calculation_type = BOTH;
        }
        else {  
            current_calculation_type = MARGIN;
        }
    }
    void Approx::setEpsilon(double epsilon) {
        CATCH_ENFORCE(epsilon >= 0 && epsilon <= 1.0,
            "Invalid Approx::epsilon: " << epsilon << '.'
            << " Approx::epsilon has to be in [0, 1]");
        m_epsilon = epsilon;
         if( current_calculation_type == MARGIN ) {
            current_calculation_type = BOTH;
        }
        else {  
            current_calculation_type = EPSILON;
        }    
    }

[chaitan94]

I am trying something similar to the following:

REQUIRE(1500000000 == Approx(1500001000).margin(60));

I noticed this evaluates to true, which I think it shouldn't. Is this a bug, or did I misunderstand margin?

For more context, I am trying to compare two unix timestamps (which are in the units of seconds) are equal, within a margin of a minute. (60 seconds).

[horenmar]

@chaitan94 Approx defaults to using some small relative margin. I don't wanna do the math, but it is likely that's what you are running into -- try adding .epsilon(0).

[chaitan94]

@horenmar You are right. Adding .epsilon(0) gets me the expected behavior. But ideally, I would expect it to work even without that. Still, thanks for pointing that out!

[oddlad]

The documentation for assertions at https://github.com/catchorg/Catch2/blob/devel/docs/assertions.md specifies the behavior for margin as follows:
margin - margin serves to set the the absolute value by which a result can differ from Approx's value before it is rejected. By default set to 0.0.

If .epsilon(0) has to be added to obtain advertised behavior, this should be reflected in the docs. Preferably, the code should be corrected to make margin() perform as documented.