D issues are now tracked on GitHub. This Bugzilla instance remains as a read-only archive.
Issue 3232 - std.math.approxEqual should consider maxAbsDiff when rhs==0 && lhs!=0
Summary: std.math.approxEqual should consider maxAbsDiff when rhs==0 && lhs!=0
Status: RESOLVED FIXED
Alias: None
Product: D
Classification: Unclassified
Component: phobos (show other issues)
Version: D2
Hardware: Other Linux
: P2 normal
Assignee: Andrei Alexandrescu
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-08-07 03:24 UTC by Lars T. Kyllingstad
Modified: 2015-06-09 01:28 UTC (History)
1 user (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this issue.
Description Lars T. Kyllingstad 2009-08-07 03:24:19 UTC
Currently, approxEqual doesn't take the maximum absolute difference into account when rhs is zero while lhs is nonzero. An example:

  double epsrel = 0.01;  // ...or whatever, it doesn't matter
  double epsabs = 1e-5; // This matters when rhs or lhs is zero!

  assert (approxEqual(0.0, 1e-10, epsrel, epsabs));  // OK
  assert (approxEqual(1e-10, 0.0, epsrel, epsabs));  // Fails!

This is very unintuitive -- I think the order of the "operands" shouldn't matter here. The offending piece of code is at line 3087 of std.math (rev. 1233):

    if (rhs == 0) {
        return (lhs == 0 ? 0 : 1) <= maxRelDiff;
    }

This could be changed to:

    if (rhs == 0) {
        return (lhs == 0 ? 0 : 1) <= maxRelDiff
            || (maxAbsDiff != 0 && fabs(rhs-lhs) <= maxAbsDiff);
    }

Another option, if abs(lhs-rhs)/rhs and abs(lhs-rhs)/lhs could be considered equally good definitions of the relative difference, would be this:

    if (rhs == 0) {
        if (lhs == 0) return true;
        // Switch lhs and rhs
        return approxEqual(rhs, lhs, maxRelDiff, maxAbsDiff);
    }

I actually prefer this one, because the name "approxEqual" doesn't in any way imply that the order of its arguments matter. It should simply return true if lhs and rhs are approximately equal, regardless of which is which.
Comment 1 Lars T. Kyllingstad 2009-08-11 00:36:02 UTC
I just realised the two solutions I suggested are exactly equivalent. :)
Comment 2 Lars T. Kyllingstad 2010-08-27 06:44:27 UTC
Fixed by Andrei a long time ago.

http://www.dsource.org/projects/phobos/changeset/1313