Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#16889 closed defect (fixed)

Doctest tolerance: allow spaces and use intervals

Reported by: jdemeyer Owned by:
Priority: blocker Milestone: sage-6.4
Component: doctest framework Keywords:
Cc: zimmerma Merged in: sage-6.4.beta3
Authors: Jeroen Demeyer Reviewers: Volker Braun
Report Upstream: N/A Work issues:
Branch: 1f858c7 Commit: 1f858c7511d147e2f93dd0fd4e160fed8ad4f234
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

The doctesting framework should ignore whitespace before numbers and between the sign and the number when testing with (abs|rel) tol. There are two reasons why:

  1. The spacing of matrices is unpredictable. You could have
    [1.0 0.0]
    [1.0 0.0]
    

vs

[0.999999 0.0]
[     1.0 0.0]

Note the added space between [ and 1.0.

  1. Complex numbers or polynomials
    1.0 + 1e-16*I
    

vs

1.0 - 1e-16*I

Here you want to compare + 1e-16 and - 1e-16 with the sign for your tolerance computations.

After some consideration, I decided it actually makes a lot of sense to use interval arithmetic for doctest tolerances, this is implemented on this ticket.

Change History (21)

comment:1 Changed 3 years ago by jdemeyer

  • Branch set to u/jdemeyer/ticket/16889
  • Created changed from 08/27/14 14:43:50 to 08/27/14 14:43:50
  • Modified changed from 08/27/14 14:43:50 to 08/27/14 14:43:50

comment:2 Changed 3 years ago by git

  • Commit set to 46301acf040394a5660c7a9a7262116cfc721804

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

46301acDoctest tolerance: allow spaces

comment:3 Changed 3 years ago by git

  • Commit changed from 46301acf040394a5660c7a9a7262116cfc721804 to 291a442c8b006a8224e76cc68df7ad9708333c96

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

291a442Doctest tolerance: allow spaces

comment:4 Changed 3 years ago by jdemeyer

  • Cc zimmerma added

Paul Zimmermann: this needs a minor change to a French book doctest. When using # tol, you may no longer split doctest results like

1.5 -
2.3

The minus sign may not be on its own line, you need to write

1.5
- 2.3

comment:5 Changed 3 years ago by jdemeyer

  • Description modified (diff)
  • Status changed from new to needs_review

comment:6 follow-up: Changed 3 years ago by zimmerma

in the source of the french book, I've changed to:

  [0.347521510119 + 0.566550553398*I, 0.347521510119 - 0.566550553398*I,
  0.345023776962 + 0.439908702386*I, 0.345023776962 - 0.439908702386*I,
  -0.517257614325 + 0.512958206789*I,
  -0.517257614325 - 0.512958206789*I, -1.36699716455, -9.98357818097]

which uses the same number of lines, and should not make any overfull hbox. Is that ok?

Paul

comment:7 in reply to: ↑ 6 Changed 3 years ago by jdemeyer

Replying to zimmerma:

in the source of the french book, I've changed to:

  [0.347521510119 + 0.566550553398*I, 0.347521510119 - 0.566550553398*I,
  0.345023776962 + 0.439908702386*I, 0.345023776962 - 0.439908702386*I,
  -0.517257614325 + 0.512958206789*I,
  -0.517257614325 - 0.512958206789*I, -1.36699716455, -9.98357818097]

which uses the same number of lines, and should not make any overfull hbox. Is that ok?

Yes, that would be good. However, note that Sage itself actually formats the results as in my patch, with one value per line.

comment:8 Changed 3 years ago by zimmerma

ok, however in our book we allow ourselves not to follow 100% the Sage output format, up to spaces and line breaks, to save some vertical space.

Paul

comment:9 Changed 3 years ago by jdemeyer

Fun fact: this ticket relies on functionality added in 2006:

  • src/sage/ext/mpfr.pyx

    commit 04043acc648e0bca14725f147b2040e204ae1f9b
    Author: William Stein <wstein@ucsd.edu>
    Date:   Mon May 29 01:10:56 2006 +0000
    
        [project @ mpfr -- can coerce in numbers ignoring space, e.g. RR('1 E7') now works (which is what pari outputs!)]
    
    diff --git a/src/sage/ext/mpfr.pyx b/src/sage/ext/mpfr.pyx
    index 8f92208..afc9b72 100644
    a b cdef class RealNumber(element.RingElement): 
    602602            d = parent(x.denominator())
    603603            mpfr_div(self.value, n.value, d.value, parent.rnd)
    604604        else:
    605             s = str(x)
     605            s = str(x).replace(' ','')
    606606            if mpfr_set_str(self.value, s, base, parent.rnd):
    607607                if s == 'NaN' or s == '@NaN@':
    608608                    mpfr_set_nan(self.value)

comment:10 Changed 3 years ago by jdemeyer

  • Description modified (diff)

comment:11 Changed 3 years ago by git

  • Commit changed from 291a442c8b006a8224e76cc68df7ad9708333c96 to dde3a3a13ad9d78afe014743d31b766e0ced2af8

Branch pushed to git repo; I updated commit sha1. New commits:

dde3a3aUse intervals for doctest tolerances

comment:12 Changed 3 years ago by jdemeyer

  • Description modified (diff)
  • Summary changed from Doctest tolerance: allow spaces to Doctest tolerance: allow spaces and use intervals

comment:13 Changed 3 years ago by git

  • Commit changed from dde3a3a13ad9d78afe014743d31b766e0ced2af8 to 59b511ea9efef56e1e9226216ff67212575e4cde

Branch pushed to git repo; I updated commit sha1. New commits:

59b511eAdd tolerance test file

comment:14 Changed 3 years ago by vbraun

This looks good to me. How about renaming the tolerance.rst to must_fail.rst or so and have an explanatory sentence about what is going on at the top. I fear that somebody will sooner or later "fix" the failing doctests if its not clear ;-)

comment:15 Changed 3 years ago by git

  • Commit changed from 59b511ea9efef56e1e9226216ff67212575e4cde to 1f858c7511d147e2f93dd0fd4e160fed8ad4f234

Branch pushed to git repo; I updated commit sha1. New commits:

1f858c7Add comment that we test failing tests

comment:16 Changed 3 years ago by vbraun

  • Reviewers set to Volker Braun
  • Status changed from needs_review to positive_review

comment:17 Changed 3 years ago by vbraun

  • Status changed from positive_review to needs_work

comment:18 Changed 3 years ago by vbraun

  • Branch changed from u/jdemeyer/ticket/16889 to u/vbraun/ticket/16889

comment:19 Changed 3 years ago by jdemeyer

  • Modified changed from 09/01/14 21:44:18 to 09/01/14 21:44:18
  • Status changed from needs_work to positive_review

comment:20 Changed 3 years ago by vbraun

  • Branch changed from u/vbraun/ticket/16889 to 1f858c7511d147e2f93dd0fd4e160fed8ad4f234
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:21 Changed 3 years ago by kcrisman

  • Merged in set to sage-6.4.beta3
Note: See TracTickets for help on using tickets.