Opened 6 years ago

Closed 3 years ago

#21121 closed defect (fixed)

Precision in doctest tolerance: x > x

Reported by: leif Owned by:
Priority: minor Milestone: sage-8.4
Component: doctest framework Keywords: doctest tolerance exceeded
Cc: Merged in:
Authors: Jeroen Demeyer Reviewers: Marc Mezzarobba
Report Upstream: N/A Work issues:
Branch: d7f0862 (Commits, GitHub, GitLab) Commit: d7f08627df8903903f2b42ed461e4ba0b477b216
Dependencies: Stopgaps:

Status badges

Description (last modified by jdemeyer)

Failed example:
    A.exp()  # tol 1e-14
Expected:
    [-19.614602953804912 + 12.517743846762578*I   3.7949636449582176 + 28.88379930658099*I]
    [ -32.383580980922254 + 21.88423595789845*I   2.269633004093535 + 44.901324827684824*I]
Got:
    [-19.614602953804923 + 12.517743846762574*I  3.7949636449582007 + 28.883799306580993*I]
    [-32.383580980922275 + 21.884235957898447*I  2.2696330040935115 + 44.901324827684846*I]
Tolerance exceeded in 1 of 8:
    2.269633004093535 vs 2.2696330040935115, tolerance 1e-14 > 1e-14

Sage saying that 10-14 > 10-14 appears a bit stupid.

The attached branch fixes this by displaying y > x where:

  • x is the required tolerance (given in the doctest) with 15 digits, rounded to nearest.
  • y is the observed tolerance (computed by running the test) with 1 digit, rounded up. This means that using # tol y is guaranteed to make the test pass.

Assuming that the tolerance in the doctest is given to at most 15 digits of precision (almost certainly the case), x should be exactly what was in the doctest. In that case, y cannot be equal to x.

We also refactor the implementation for printing tolerance errors somewhat.

Finally, we use 1044 bits of precision to compute tolerances. This may seem overkill, but it is needed if we want to deal with tolerances on numbers computed with 1024 bits of precision.

Change History (28)

comment:1 Changed 6 years ago by jdemeyer

  • Dependencies set to #21124

comment:2 Changed 6 years ago by leif

  • Description modified (diff)

comment:3 Changed 6 years ago by leif

The culprit (at least regarding the weird ouput) is here, namely in the local function failstr():

    def output_difference(self, example, got, optionflags):
        r""" ... """
        got = self.human_readable_escape_sequences(got)
        want = example.want
        diff = doctest.OutputChecker.output_difference(self, example, got, optionflags)
        if isinstance(want, MarkedOutput) and (want.tol or want.abs_tol or want.rel_tol):
            if diff[-1] != "\n":
                diff += "\n"
            want_str = [g[0] for g in float_regex.findall(want)]
            got_str = [g[0] for g in float_regex.findall(got)]
            want_values = [RIFtol(g) for g in want_str]
            want_intervals = [self.add_tolerance(v, want) for v in want_values]
            got_values = [RIFtol(g) for g in got_str]
            if len(want_values) == len(got_values):
                def failstr(astr, bstr, actual, desired):
                    return "    %s vs %s, tolerance %.0e > %.0e"%(astr, bstr, RIFtol(actual).center(), RIFtol(desired).center())

                fails = []
                for a, ainterval, b, astr, bstr in zip(want_values, want_intervals, got_values, want_str, got_str):
                    if not ainterval.overlaps(b):
                        if want.tol:
                            if a == 0:
                                fails.append(failstr(astr, bstr, abs(b), want.tol))
                            else:
                                fails.append(failstr(astr, bstr, abs(1 - b/a), want.tol))
                        elif want.abs_tol:
                            fails.append(failstr(astr, bstr, abs(a - b), want.abs_tol))
                        else:
                            fails.append(failstr(astr, bstr, abs(1 - b/a), want.rel_tol))

                if fails:
                    if len(want_values) == 1:
                        diff += "Tolerance exceeded:\n"
                    else:
                        diff += "Tolerance exceeded in %s of %s:\n"%(len(fails), len(want_values))
                    diff += "\n".join(fails) + "\n"
        return diff

(A method of class SageOutputChecker in src/sage/doctest/parsing.py.)

One could also change the code below its definition rather than (just) failstr().

comment:4 Changed 6 years ago by leif

(As is, failstr() truncates the mantissa to just one digit, i.e., the digit before the decimal point.)

comment:5 Changed 6 years ago by leif

I.e., we should at least make sure "%.0e" % RIFtol(actual).center() doesn't give the same as "%.0e" % RIFtol(desired).center(). (Not sure whether using .center() here is at all the right thing, or whether we should at all use RIF for the tolerance values themselves; see also below.)

If it does, we may increase the number of (mantissa) digits in the former. Or in both, until they differ.

And that's orthogonal to improving hints regarding the actual deviation, i.e., if one wants to adjust the expected figures rather than (just) the tolerance.

If the intervals of want and got do not overlap (i.e., when the tolerance is exceeded), we could also give .upper() and .lower() (or vice versa, depending on whether want is smaller or larger than got).

comment:6 Changed 6 years ago by leif

  • Description modified (diff)

comment:7 Changed 6 years ago by leif

With

  • src/sage/doctest/parsing.py

    diff --git a/src/sage/doctest/parsing.py b/src/sage/doctest/parsing.py
    index b297bb1..def1e31 100644
    a b class SageOutputChecker(doctest.OutputChecker): 
    963963            want_intervals = [self.add_tolerance(v, want) for v in want_values]
    964964            got_values = [RIFtol(g) for g in got_str]
    965965            if len(want_values) == len(got_values):
     966
    966967                def failstr(astr, bstr, actual, desired):
    967                     return "    %s vs %s, tolerance %.0e > %.0e"%(astr, bstr, RIFtol(actual).center(), RIFtol(desired).
     968                    # Do not print 'x > x' (#21121):
     969                    dp = 0 # start with zero decimal places in output of tolerance
     970                    while True: # XXX kind of dangerous...
     971                        allowed_tol = "%.*e" % (dp, RIFtol(desired).center())
     972                        effective_tol = "%.*e" % (dp, RIFtol(actual).center())
     973                        if allowed_tol != effective_tol:
     974                            # we achieved meaningful output
     975                            break
     976                        dp += 1 # increase number of decimal places to be shown
     977
     978                    return "    %s vs %s, tolerance %s > %s"%(astr, bstr, effective_tol, allowed_tol)
    968979
    969980                fails = []
    970981                for a, ainterval, b, astr, bstr in zip(want_values, want_intervals, got_values, want_str, got_str):

I would for example get the following for the failing test from the ticket's description / #21119:

sage -t src/sage/matrix/matrix_double_dense.pyx
**********************************************************************
File "src/sage/matrix/matrix_double_dense.pyx", line 3761, in sage.matrix.matrix_double_dense.Matrix_double_dense.exp
Failed example:
    A.exp()  # tol 1e-14
Expected:
    [-19.614602953804912 + 12.517743846762578*I   3.7949636449582176 + 28.88379930658099*I]
    [ -32.383580980922254 + 21.88423595789845*I   2.269633004093535 + 44.901324827684824*I]
Got:
    [-19.614602953804923 + 12.517743846762574*I  3.7949636449582007 + 28.883799306580993*I]
    [-32.383580980922275 + 21.884235957898447*I  2.2696330040935115 + 44.901324827684846*I]
Tolerance exceeded in 1 of 8:
    2.269633004093535 vs 2.2696330040935115, tolerance 1.04e-14 > 1.00e-14
**********************************************************************

AFAICT, we unfortunately there do not know whether # tol 1e-14 or e.g. # tol 1.0e-14 was specified in the failing doctest.

comment:8 Changed 6 years ago by leif

To do:

Replace True by dp <= some_meaningful_number and break by return ... (returning what's now returned below, after the loop).

Then if we exit the while loop (giving up on too many decimal places), fall back to returning some other (meaningful) output, of course again other than ... x > x.

comment:9 Changed 6 years ago by leif

... and the mentioned doctests for the doctest framework itself of course need to get adjusted to the new output.

comment:10 Changed 6 years ago by jdemeyer

You just need to print the numbers with directed roundings (up/down). But that needs #21124 to properly print the numbers.

comment:11 Changed 4 years ago by jdemeyer

  • Branch set to u/jdemeyer/precision_in_doctest_tolerance__x___x

comment:12 Changed 4 years ago by jdemeyer

  • Commit set to a9909ab60eb35282534e545ee1e7ec633601c779
  • Dependencies changed from #21124 to #26154

New commits:

a9909abImprove output formatting for tolerances

comment:13 Changed 4 years ago by git

  • Commit changed from a9909ab60eb35282534e545ee1e7ec633601c779 to e1ba5b73f39a47446d01581550ccf2a4259f14ca

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

7fcc109Omit point when printing real numbers in scientific notation with 1 digit
e1ba5b7Improve output formatting for tolerances

comment:14 Changed 4 years ago by git

  • Commit changed from e1ba5b73f39a47446d01581550ccf2a4259f14ca to a636d1ee3c389f7270652db664e4b5ecfc388fd2

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

a636d1eImprove output formatting for tolerances

comment:15 Changed 4 years ago by jdemeyer

  • Authors set to Jeroen Demeyer
  • Description modified (diff)
  • Keywords tolerance added; error removed
  • Milestone changed from sage-7.4 to sage-8.4
  • Status changed from new to needs_review

comment:16 Changed 4 years ago by jdemeyer

  • Description modified (diff)

comment:17 Changed 4 years ago by git

  • Commit changed from a636d1ee3c389f7270652db664e4b5ecfc388fd2 to cb741123f8dec1849052c88b47dbc50caf3f6ab1

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

cb74112Improve output formatting for tolerances

comment:18 Changed 4 years ago by jdemeyer

  • Description modified (diff)

comment:19 Changed 4 years ago by jdemeyer

  • Description modified (diff)

comment:20 Changed 4 years ago by git

  • Commit changed from cb741123f8dec1849052c88b47dbc50caf3f6ab1 to dd60a2e10199532a02bf591a41644df64d9ebef6

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

dd60a2eImprove output formatting for tolerances

comment:21 Changed 4 years ago by git

  • Commit changed from dd60a2e10199532a02bf591a41644df64d9ebef6 to 5535d2de60d94021d0332369e9cbe9ddc9e32cfd

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

5535d2dImprove output formatting for tolerances

comment:22 Changed 3 years ago by git

  • Commit changed from 5535d2de60d94021d0332369e9cbe9ddc9e32cfd to b76623dc03e4d81ff303eac219ad5c907a4c2360

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

b76623dImprove output formatting for tolerances

comment:23 Changed 3 years ago by jdemeyer

  • Dependencies #26154 deleted

comment:24 Changed 3 years ago by git

  • Commit changed from b76623dc03e4d81ff303eac219ad5c907a4c2360 to e62891b0edc5754465df192ebb174938a7706e9e

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

e62891bImprove output formatting for tolerances

comment:25 Changed 3 years ago by mmezzarobba

  • Reviewers set to Marc Mezzarobba
  • Status changed from needs_review to positive_review

LGTM (except that “it is carries” in the comment should be “it carries”).

comment:26 Changed 3 years ago by git

  • Commit changed from e62891b0edc5754465df192ebb174938a7706e9e to d7f08627df8903903f2b42ed461e4ba0b477b216
  • Status changed from positive_review to needs_review

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. This was a forced push. New commits:

d7f0862Improve output formatting for tolerances

comment:27 Changed 3 years ago by jdemeyer

  • Status changed from needs_review to positive_review

Fixed typo

comment:28 Changed 3 years ago by vbraun

  • Branch changed from u/jdemeyer/precision_in_doctest_tolerance__x___x to d7f08627df8903903f2b42ed461e4ba0b477b216
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.