Opened 4 years ago
Closed 4 years ago
#26019 closed defect (fixed)
py3: fixes to sage.rings.polynomial.real_roots
Reported by: | embray | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-8.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, GitHub, GitLab) | Commit: | db22bc7951f4a054166b99f5cb8f74af587f178f |
Dependencies: | Stopgaps: |
Description
This fixes all the tests for this module on Python 3, and also serves as a small follow-up to #25247.
Change History (8)
comment:1 Changed 4 years ago by
- Status changed from new to needs_review
comment:2 Changed 4 years ago by
- Reviewers set to Jeroen Demeyer
- Status changed from needs_review to positive_review
comment:3 Changed 4 years ago by
- Status changed from positive_review to needs_work
comment:4 Changed 4 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 4 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 4 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 non-obvious, 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 re-seeding it, which could just as well be avoided by the addition of a new optional argument to randstate.python_random()
.
comment:7 Changed 4 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 4 years ago by
- Branch changed from u/embray/python3/sage-rings-polynomial-real_roots/misc to db22bc7951f4a054166b99f5cb8f74af587f178f
- Resolution set to fixed
- Status changed from positive_review to closed
See patchbot