Bug 15881 – approxEqual Ignores maxAbsDiff

The current implementation of approxEqual will ignore the maxAbsDiff term in most circumstances. The code below compares two floating point numbers. They are sufficiently far away that on an absolute basis the assertion should fail. However, on a relative basis, there is less than a 1% difference between the two. import std.math : approxEqual, fabs; void main() { auto x = 1000.0; auto y = x + 10.0; assert(approxEqual(x, y)); //should fail assert(fabs((x - y) / y) <= 1E-2); assert(!(fabs(x - y) <= 1E-5)); assert(!(fabs((x - y) / y) <= 1E-2 && fabs(x - y) <= 1E-5)); assert(fabs((x - y) / y) <= 1E-2 || 1E-5 != 0 && fabs(x - y) <= 1E-5); //this is effectively the current implementation, wrong! } The exact line of the approxEqual code causing the problem is return fabs((lhs - rhs) / rhs) <= maxRelDiff || maxAbsDiff != 0 && fabs(lhs - rhs) <= maxAbsDiff; It mirrors the last assert in the code above. The || operator short-circuits the last part of the logical statement from being called. Perhaps changing it to return fabs((lhs - rhs) / rhs) <= maxRelDiff && maxAbsDiff != 0 && fabs(lhs - rhs) <= maxAbsDiff; will resolve the issue. Alternately, return (fabs((lhs - rhs) / rhs) <= maxRelDiff) | (maxAbsDiff != 0) && fabs(lhs - rhs) <= maxAbsDiff; might resolve the issue.

Actually, after thinking about it more, it should probably be something like if (maxAbsDiff >= 0) return (fabs((lhs - rhs) / rhs) <= maxRelDiff) && fabs(lhs - rhs) <= maxAbsDiff; else assert(0, "Error: maxAbsDiff must be greater than or equal to zero"); This may not be exactly what the original authors intended, but it makes the most sense to me. I had also considered making (maxAbsDiff >= 0) to be (maxAbsDiff > 0). The downside is that this is enforcing them to be exactly equal, but that might be something worth doing.

Ok, so I just ran in to this with approxEqual(2531.25, 2531) == true, which was a latent bug hiding in my code for over a year when I thought everything was working fine. I would like to get some more opinions from others on this: either we change it so both relative *and* absolute must be satisfied, or it should be very clearly documented to only need to satisfy one of them.

If I were designing it from the start, I probably would have used a function like bool approxEqual(T, U, V)(T lhs, U rhs, V maxDiff, bool isAbsolute = FALSE); This way you can easily switch from one to the other. It wouldn't break any code to add this as an additional option. The other one could be fixed so as to match the documentation. This would mean that there is the simple one that's an OR and another version that can handle the AND case (which some people still might need).

Christopher Barker has collected a lot of thoughts on this theme in a proposal for a similar function in python (called isclose there) [1]. The test implementation with several versions for comparison can be found on github [2]. According to Barker the or-ing of the two tests is not the problem, but the large default value for maxRelDiff (1e-2). Barker argues to use 1e-9 for python floats (which seem to be D's doubles). Additionally the function should be symmetric, i.e. the order of the first two arguments should not matter. At present approxEqual(0.99001,1) != approxEqual(1,0.99001). This is mainly for not confusing users. Barker suggests to use 0.0 for the default of maxAbsDiff, because it is completely dependend on the usecase, and other defaults would be missleading. People should think about what limit they need and not rely on a given default, which may or may not be suitable. Implementing this in Phobos will lead to lots of unittests failing, because they rely on the actual implementation. Anyway, I think, we should follow Barkers thoughts. I my oppinion they have their merits. PS: It would be nice to have different default values for maxRelDiff, depending on which floatingpoint numbers are used. But I don't know, if this is easy to implement. [1] https://www.python.org/dev/peps/pep-0485/ [2] https://github.com/PythonCHB/close_pep/blob/master/is_close.py

@crocopaw created dlang/phobos pull request #7173 "Fix Issue 15881 - approxEqual Ignores maxAbsDiff" fixing this issue: - Fix issue 15881 - approxEqual Ignores maxAbsDiff Moved approxEqual2 to approxEqual and adapted documentation. https://github.com/dlang/phobos/pull/7173

See PR https://github.com/dlang/phobos/pull/7180 for making clearer, what the function does. Not sure if this might already be accepted as a bugfix.