https://issues.dlang.org/show_bug.cgi?id=15881

--- Comment #1 from John Hall <john.michael.hall@gmail.com> ---
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.

--

https://issues.dlang.org/show_bug.cgi?id=15881

greensunny12@gmail.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |greensunny12@gmail.com

--

https://issues.dlang.org/show_bug.cgi?id=15881

John Colvin <john.loughran.colvin@gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |john.loughran.colvin@gmail.
                   |                            |com

--- Comment #2 from John Colvin <john.loughran.colvin@gmail.com> ---
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.

--

https://issues.dlang.org/show_bug.cgi?id=15881

--- Comment #3 from John Hall <john.michael.hall@gmail.com> ---
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).

--

https://issues.dlang.org/show_bug.cgi?id=15881

Berni <dlang@croco-puzzle.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |dlang@croco-puzzle.com

--- Comment #4 from Berni <dlang@croco-puzzle.com> ---
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

--

https://issues.dlang.org/show_bug.cgi?id=15881

Dlang Bot <dlang-bot@dlang.rocks> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |pull

--- Comment #5 from Dlang Bot <dlang-bot@dlang.rocks> ---
@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

--

https://issues.dlang.org/show_bug.cgi?id=15881

--- Comment #6 from Berni <dlang@croco-puzzle.com> ---
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.

--

https://issues.dlang.org/show_bug.cgi?id=15881

berni44 <bugzilla@d-ecke.de> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
                 CC|                            |bugzilla@d-ecke.de
         Resolution|---                         |FIXED

--