Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sage 3.3: numerical noise in rings/polynomial/complex_roots.py on cicero & fulvia #5378

Closed
sagetrac-mabshoff mannequin opened this issue Feb 26, 2009 · 7 comments
Closed

Comments

@sagetrac-mabshoff
Copy link
Mannequin

sagetrac-mabshoff mannequin commented Feb 26, 2009

sage -t  "devel/sage/sage/rings/polynomial/complex_roots.py"
**********************************************************************
File "/home/mariah/sage/sage-3.3-x86-Linux-fc-test/devel/sage/sage/rings/polynom
ial/complex_roots.py", line 271:
   sage: complex_roots(x^2 + 27*x + 181)
Expected:
   [(-14.61803398874990?..., 1), (-12.3819660112501...? + 0.?e-27*I, 1)]
Got:
   [(-12.3819660112501?, 1), (-14.61803398874990? + 0.?e-27*I, 1)]

CC: @sagetrac-cwitty

Component: doctest coverage

Issue created by migration from https://trac.sagemath.org/ticket/5378

@sagetrac-mabshoff sagetrac-mabshoff mannequin added this to the sage-3.4 milestone Feb 26, 2009
@sagetrac-mabshoff sagetrac-mabshoff mannequin self-assigned this Feb 26, 2009
@sagetrac-mabshoff
Copy link
Mannequin Author

sagetrac-mabshoff mannequin commented Mar 1, 2009

comment:1

Hmm, the numerical noise of the imaginary part of the root causes the order of the roots for printing to be flipped. I am not sure what to do here except for picking another polynomial, but I have not looked into this in any detail since we might have this particular root for a good reason.

Cheers,

Michael

@sagetrac-mabshoff sagetrac-mabshoff mannequin modified the milestones: sage-3.4, sage-3.4.1 Mar 1, 2009
@sagetrac-cwitty
Copy link
Mannequin

sagetrac-cwitty mannequin commented Mar 1, 2009

comment:2

I don't remember anything special about that polynomial, so I'm fine with changing it.

Other possibilities would include changing the sorting. One possibility would be to remove the code that puts real roots first; another possibility would be to special-case complex interval roots in the sorting, and say that if the imaginary part of a root is an interval that contains 0 then it should sort with the real roots.

@sagetrac-mabshoff
Copy link
Mannequin Author

sagetrac-mabshoff mannequin commented Apr 18, 2009

comment:3

Move this to 3.4.1 since I am closing this as dupe of #5559.

Cheers,

Michael

@sagetrac-mabshoff sagetrac-mabshoff mannequin modified the milestones: sage-3.4.2, sage-3.4.1 Apr 18, 2009
@sagetrac-mabshoff
Copy link
Mannequin Author

sagetrac-mabshoff mannequin commented Apr 19, 2009

comment:4

The following illustrates the problem and a potential solution:

----------------------------------------------------------------------
| Sage Version 3.4.1.rc3, Release Date: 2009-04-16                   |
| Type notebook() for the GUI, and license() for information.        |
----------------------------------------------------------------------
Loading Sage library. Current Mercurial branch is: review
sage: from sage.rings.polynomial.complex_roots import complex_roots
sage: x = polygen(ZZ) 
sage: complex_roots(x^2 + 27*x + 181)
[(-12.3819660112501?, 1), (-14.61803398874990? + 0.?e-27*I, 1)]
sage: v=complex_roots(x^2 + 27*x + 181)
sage: sorted((v[0][0].real(),v[1][0].real()))
[-14.61803398874990?, -12.3819660112501?]

On another machine we get:

sage: from sage.rings.polynomial.complex_roots import complex_roots
sage:  x = polygen(ZZ)  
sage: complex_roots(x^2 + 27*x + 181)
[(-14.61803398874990? + 0.?e-27*I, 1), (-12.38196601125010? + 0.?e-27*I, 1)]
sage: v=complex_roots(x^2 + 27*x + 181)
sage: sorted((v[0][0].real(),v[1][0].real()))
[-14.61803398874990?, -12.38196601125010?]

Patch coming up.

Cheers,

Michael

@sagetrac-mabshoff
Copy link
Mannequin Author

sagetrac-mabshoff mannequin commented Apr 19, 2009

Attachment: trac_5378.patch.gz

@rbeezer
Copy link
Mannequin

rbeezer mannequin commented Apr 19, 2009

comment:6

Builds and tests just fine. Positive review.

@sagetrac-mabshoff
Copy link
Mannequin Author

sagetrac-mabshoff mannequin commented Apr 19, 2009

comment:7

Merged in Sage 3.4.1.rc4.

Cheers,

Michael

@sagetrac-mabshoff sagetrac-mabshoff mannequin closed this as completed Apr 19, 2009
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

0 participants