Opened 2 years ago
Closed 2 years ago
#26019 closed defect (fixed)
py3: fixes to sage.rings.polynomial.real_roots
Reported by:  embray  Owned by:  

Priority:  major  Milestone:  sage8.4 
Component:  python3  Keywords:  
Cc:  Merged in:  
Authors:  Erik Bray  Reviewers:  Jeroen Demeyer, Frédéric Chapoton 
Report Upstream:  N/A  Work issues:  
Branch:  db22bc7 (Commits)  Commit:  db22bc7951f4a054166b99f5cb8f74af587f178f 
Dependencies:  Stopgaps: 
Description
This fixes all the tests for this module on Python 3, and also serves as a small followup to #25247.
Change History (8)
comment:1 Changed 2 years ago by
 Status changed from new to needs_review
comment:2 Changed 2 years ago by
 Reviewers set to Jeroen Demeyer
 Status changed from needs_review to positive_review
comment:3 Changed 2 years ago by
 Status changed from positive_review to needs_work
comment:4 Changed 2 years ago by
For the record:
sage t long src/sage/schemes/elliptic_curves/ell_rational_field.py ********************************************************************** File "src/sage/schemes/elliptic_curves/ell_rational_field.py", line 3338, in sage.schemes.elliptic_curves.ell_rational_field.EllipticCurve_rational_field.elliptic_exponential Failed example: 2 * E.elliptic_exponential(z) Expected: (1.52184235874404  0.0581413944316544*I : 0.948655866506124  0.0381469928565030*I : 1.00000000000000) Got: (0.379082374337223 + 0.401554153021658*I : 1.64457248125273 + 0.436489289368578*I : 1.00000000000000) ********************************************************************** File "src/sage/schemes/elliptic_curves/ell_rational_field.py", line 3340, in sage.schemes.elliptic_curves.ell_rational_field.EllipticCurve_rational_field.elliptic_exponential Failed example: E.elliptic_exponential(2 * z) Expected: (1.52184235874404  0.0581413944316562*I : 0.948655866506128  0.0381469928565034*I : 1.00000000000000) Got: (0.379082374337223 + 0.401554153021660*I : 1.64457248125273 + 0.436489289368575*I : 1.00000000000000) ********************************************************************** 1 item had failures: 2 of 21 in sage.schemes.elliptic_curves.ell_rational_field.EllipticCurve_rational_field.elliptic_exponential [848 tests, 2 failures, 176.91 s]  sage t long src/sage/schemes/elliptic_curves/ell_rational_field.py # 2 doctests failed 
possibly related to the randomization change but that's not clear yet.
comment:5 Changed 2 years ago by
 Commit changed from bec801e3b085e866a88906561c912d6b03ef52a3 to db22bc7951f4a054166b99f5cb8f74af587f178f
Branch pushed to git repo; I updated commit sha1. New commits:
db22bc7  add an optional 'seed' argument to randstate.python_random

comment:6 Changed 2 years ago by
 Status changed from needs_work to needs_review
This should fix it. A just as valid fix would have been to change the expected output of the tests that broke, which only broke due to a valid, but nonobvious, change in the PRNG sequence due to adding a call to randstate.python_random()
, which in turn calls ZZ.random_element()
.
So technically nothing was actually broken. Nevertheless, it occurred to me that in this case we were seeding the Python Random
with ZZ.random_element()
, and then immediately reseeding it, which could just as well be avoided by the addition of a new optional argument to randstate.python_random()
.
comment:7 Changed 2 years ago by
 Reviewers changed from Jeroen Demeyer to Jeroen Demeyer, Frédéric Chapoton
 Status changed from needs_review to positive_review
ok, then
comment:8 Changed 2 years ago by
 Branch changed from u/embray/python3/sageringspolynomialreal_roots/misc to db22bc7951f4a054166b99f5cb8f74af587f178f
 Resolution set to fixed
 Status changed from positive_review to closed
See patchbot