Opened 10 years ago

Closed 8 years ago

#8720 closed defect (fixed)

CC and CDF do not display numeric 0

Reported by: jason Owned by: AlexGhitza
Priority: major Milestone: sage-5.0
Component: basic arithmetic Keywords:
Cc: zimmerma, mhansen Merged in: sage-5.0.beta14
Authors: Jason Grout, Mike Hansen, Paul Zimmermann Reviewers: Paul Zimmermann, Karl-Dieter Crisman
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by zimmerma)

Look at the inconsistency:

sage: RR(0)
0.000000000000000
sage: RDF(0)
0.0

versus

sage: CDF(0)
0
sage: CC(0)
0

Apply trac-8720-printing-complex-zero.patch, trac_8720-doctests.patch, trac_8720-doctests-2.patch and trac_8720-doctests-3.patch.

Attachments (4)

trac-8720-printing-complex-zero.patch (1.6 KB) - added by mhansen 8 years ago.
trac_8720-doctests.patch (34.8 KB) - added by mhansen 8 years ago.
trac_8720-doctests-2.patch (1.5 KB) - added by mhansen 8 years ago.
trac_8720-doctests-3.patch (2.8 KB) - added by zimmerma 8 years ago.
apply this patch on top of the other three

Download all attachments as: .zip

Change History (34)

comment:1 Changed 10 years ago by jason

  • Authors set to Jason Grout
  • Cc zimmerma added

I think CDF and CC should display the same output as RDF and RR, respectively.

CCing zimmerma since he may be interested in reviewing this.

comment:2 Changed 10 years ago by zimmerma

  • Status changed from new to needs_work

since I was asked to review this ticket, I first notice that some doctests do not pass any more, for example:

sage -t  gsl/dft.py
**********************************************************************
File "/usr/local/sage-4.3.5-core2/devel/sage-8720/sage/gsl/dft.py", line 528:
    sage: t = s.fft(); t
Expected:
    Indexed sequence: [5.00000000000000, 0, 0, 0, 0]
     indexed by [0, 1, 2, 3, 4]
Got:
    Indexed sequence: [5.00000000000000, 0.000000000000000, 0.000000000000000, \
0.000000000000000, 0.000000000000000]
        indexed by [0, 1, 2, 3, 4]

thus some more work is needed. Please check all doctests still pass.

comment:3 Changed 10 years ago by jason

Thanks for looking at it. When I ran the doctests, a number of them failed, so I posted to sage-devel and kept this ticket as "needs work". I will set it to "needs review" when I've taken care of the failing doctests.

Thanks again.

comment:4 Changed 8 years ago by mhansen

I'm going to run the tests shortly and produce an doctest fixing patch.

comment:5 Changed 8 years ago by mhansen

  • Cc mhansen added

comment:6 Changed 8 years ago by mhansen

  • Status changed from needs_work to needs_review

I've attached a patch which should fix all of the doctests in 4.8.alpha4.

comment:7 Changed 8 years ago by zimmerma

note that a consequence of that ticket is that coefficients 0 that were not displayed in Taylor series are now displayed, for example:

sage: E = EllipticCurve('37a')
sage: L = E.lseries().dokchitser()
sage: L.taylor_series(1,4)
0.0000000000000 + 0.305999773834052*z + 0.186547797268162*z^2 - 0.136791463097188*z^3 + O(z^4)

(Before this ticket, the leading 0.000000000000000 was not printed.)

I find it good, since those 0.0 values can come from numerical cancellation.

Paul

comment:8 follow-up: Changed 8 years ago by zimmerma

  • Authors changed from Jason Grout to Jason Grout, Mike Hansen
  • Reviewers set to Paul Zimmermann

all tests work on top of Sage 4.7.2, I give a positive review.

Paul

comment:9 in reply to: ↑ 8 Changed 8 years ago by kcrisman

  • Status changed from needs_review to positive_review

all tests work on top of Sage 4.7.2, I give a positive review.

Just doing the radio button for this. Nice teamwork on this!

comment:10 Changed 8 years ago by jdemeyer

  • Status changed from positive_review to needs_work

A few more doctests need to be fixed:

sage -t  -force_lib devel/sage/sage/matrix/matrix2.pyx
**********************************************************************
File "/mnt/usb1/scratch/jdemeyer/merger/sage-4.8.alpha6/devel/sage-main/sage/matrix/matrix2.pyx", line 7987:
    sage: M.round(6).zero_at(10^-6)
Expected:
    [             -1.528503                      0                      0]
    [  0.459974 - 0.40061*I              -1.741233                      0]
    [-0.934304 + 0.148868*I   0.54833 + 0.073202*I              -0.550725]
Got:
    [             -1.528503                    0.0                    0.0]
    [  0.459974 - 0.40061*I              -1.741233                    0.0]
    [-0.934304 + 0.148868*I   0.54833 + 0.073202*I              -0.550725]
**********************************************************************
File "/mnt/usb1/scratch/jdemeyer/merger/sage-4.8.alpha6/devel/sage-main/sage/matrix/matrix2.pyx", line 7991:
    sage: (A - M*G).zero_at(10^-12)
Expected:
    [0 0 0]
    [0 0 0]
    [0 0 0]
Got:
    [0.0 0.0 0.0]
    [0.0 0.0 0.0]
    [0.0 0.0 0.0]
**********************************************************************
File "/mnt/usb1/scratch/jdemeyer/merger/sage-4.8.alpha6/devel/sage-main/sage/matrix/matrix2.pyx", line 7995:
    sage: (G*G.conjugate().transpose()).zero_at(10^-12)
Expected:
    [1.0   0   0]
    [  0 1.0   0]
    [  0   0 1.0]
Got:
    [1.0 0.0 0.0]
    [0.0 1.0 0.0]
    [0.0 0.0 1.0]
**********************************************************************

comment:11 follow-up: Changed 8 years ago by zimmerma

sorry, but there is no such doctest in 4.7.2. There must be some interaction with some other patch which you included. My positive review was valid for 4.7.2 only.

Paul

comment:12 in reply to: ↑ 11 Changed 8 years ago by kcrisman

Replying to zimmerma:

sorry, but there is no such doctest in 4.7.2. There must be some interaction with some other patch which you included. My positive review was valid for 4.7.2 only.

Probably Jeroen is referring to doctests in the latest alpha, on which (for better or for worse) patches need to be based on.

comment:13 Changed 8 years ago by jdemeyer

* bump *

comment:14 Changed 8 years ago by mhansen

  • Status changed from needs_work to needs_review

I posted a patch fixing these problems.

comment:15 Changed 8 years ago by kcrisman

  • Description modified (diff)

Note that the second patch applies with a little fuzz.

$ hg qpush
applying trac_8720-doctests.patch
patching file sage/matrix/matrix_double_dense.pyx
Hunk #1 succeeded at 189 with fuzz 2 (offset 9 lines).
now at: trac_8720-doctests.patch

I'll run tests and check. I should have set sage -b to do parallel building

comment:16 Changed 8 years ago by kcrisman

{{[

Doctesting 1 file using 1 thread sage -t devel/sage-main/sage/matrix/matrix_double_dense.pyx

[19.7 s]

}}}

Now running full long doctests.

comment:17 Changed 8 years ago by kcrisman

  • Reviewers changed from Paul Zimmermann to Paul Zimmermann, Karl-Dieter Crisman
  • Status changed from needs_review to needs_work
$ ../../sage -tp 4 -long sage/lfunctions/lcalc.py 
Global iterations: 1
File iterations: 1
Using long cached timings to run longest doctests first.
Doctesting 1 file using 1 thread
sage -t -long devel/sage-main/sage/lfunctions/lcalc.py
  ***   Warning: new stack size = 1028976 (0.981 Mbytes).
  ***   Warning: new stack size = 1003360 (0.957 Mbytes).
  ***   Warning: new stack size = 1001472 (0.955 Mbytes).
**********************************************************************
File "/Users/.../sage-5.0.beta2/devel/sage-main/sage/lfunctions/lcalc.py", line 229:
    sage: E.lseries().values_along_line(0.5, 3, 5)
Expected:
    [(0.000000000, 0.209951303), (0.500000000, -2.92081722e-16), (1.00000000, 0.133768433), (1.50000000, 0.360092864), (2.00000000, 0.552975867)]
Got:
    [(0.000000000, 0.209951303), (0.500000000, -2.92081723e-16), (1.00000000, 0.133768433), (1.50000000, 0.360092864), (2.00000000, 0.552975867)]
**********************************************************************
1 items had failures:
   1 of   7 in __main__.example_5
***Test Failed*** 1 failures.
For whitespace errors, see the file /Users/.../.sage/tmp/new_host_2.home-37994/lcalc_37997.py
	 [3.0 s]

The difference was in the e-16 one. I think there is a reason that one used to be -...e-16 in the old doctest. I'm going to ignore the stack size warnings, since they don't seem to have affected the outcome, though I'm sure they're not ideal.

All else is fine!

comment:18 follow-up: Changed 8 years ago by jason

Can you double-check to make sure that test was passing before the patches? It seems odd to me that error would have been triggered by this patch.

comment:19 in reply to: ↑ 18 Changed 8 years ago by kcrisman

Can you double-check to make sure that test was passing before the patches? It seems odd to me that error would have been triggered by this patch.

Notice that the test now fails because we used to ignore all digits before the exponent, not because of the stack thing - the doc even points out that warnings might happen, it's no big deal for now.

        Sometimes warnings are printed (by lcalc) when this command is
        run::
        
            sage: E = EllipticCurve('389a')
            sage: E.lseries().values_along_line(0.5, 3, 5)
            [(0, 0.209951303),
             (0.500000000, -...e-16),
             (1.00000000, 0.133768433),
             (1.50000000, 0.360092864),
             (2.00000000, 0.552975867)]

Here is 5.0.beta1:

$ ./sage -t -long devel/sage/sage/lfunctions/lcalc.py 
sage -t -long "devel/sage/sage/lfunctions/lcalc.py"         
	 [20.4 s]
 
----------------------------------------------------------------------
All tests passed!
Total time for all tests: 20.5 seconds
<snip>
sage:             sage: E = EllipticCurve('389a')
sage:             sage: E.lseries().values_along_line(0.5, 3, 5)
  ***   Warning: new stack size = 1003360 (0.957 Mbytes).
[(0, 0.209951303), (0.500000000, -2.92081723e-16), (1.00000000, 0.133768433), (1.50000000, 0.360092864), (2.00000000, 0.552975867)]

so we can see that all that has to be done is go back to the previous doctest for that one number (not even the whole list).

comment:20 Changed 8 years ago by jason

Ah, I understand. Mike made this test much more rigid, and that is what is failing here. Yeah, I agree with your conclusion, Karl-Dieter.

comment:21 Changed 8 years ago by mhansen

  • Status changed from needs_work to needs_review

I've gone ahead and fixed that doctest.

comment:22 Changed 8 years ago by davidloeffler

  • Status changed from needs_review to needs_work
  • Work issues set to rebase to current beta

I'm afraid that, according to the patchbot, this new patch works on latest release (4.8) but fails a doctest on 5.0.beta3, and doesn't even apply to 5.0.beta11.

Changed 8 years ago by mhansen

Changed 8 years ago by mhansen

Changed 8 years ago by mhansen

comment:23 Changed 8 years ago by mhansen

  • Status changed from needs_work to needs_review
  • Work issues rebase to current beta deleted

I've rebased the files on beta11.

comment:24 Changed 8 years ago by davidloeffler

  • Status changed from needs_review to needs_work
  • Work issues set to fix doctests

The beta11 patchbot brings up some doctest failures:

The following tests failed:

	sage -t  -force_lib devel/sage-8720/sage/plot/hyperbolic_triangle.py # 2 doctests failed
	sage -t  -force_lib devel/sage-8720/sage/plot/hyperbolic_arc.py # 1 doctests failed
	sage -t  -force_lib devel/sage-8720/sage/matrix/matrix_space.py # 1 doctests failed
	sage -t  -force_lib devel/sage-8720/sage/matrix/constructor.py # 1 doctests failed

Changed 8 years ago by zimmerma

apply this patch on top of the other three

comment:25 Changed 8 years ago by zimmerma

  • Authors changed from Jason Grout, Mike Hansen to Jason Grout, Mike Hansen, Paul Zimmermann
  • Status changed from needs_work to needs_review

I have attached a patch for sage-5.0.beta11, which makes the 4 doctests from comment 24 work. Note that the change in matrix/constructor.py seems to indicate it was not working properly before this ticket. Please review.

Paul

comment:26 Changed 8 years ago by zimmerma

  • Description modified (diff)

changed description for the patchbot.

Paul

comment:27 Changed 8 years ago by kcrisman

Seems to work fine on these files. Running long doctests now but I don't anticipate any more problems.

As to constructor.py, the -0.1 was removed in the first doctest patch, so the problem was in the test, not the behavior.

comment:28 Changed 8 years ago by kcrisman

  • Status changed from needs_review to positive_review

comment:29 Changed 8 years ago by kcrisman

  • Work issues fix doctests deleted

comment:30 Changed 8 years ago by jdemeyer

  • Merged in set to sage-5.0.beta14
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.