#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) 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 11 months ago by embray

  • Status changed from new to needs_review

comment:2 Changed 11 months ago by jdemeyer

  • Reviewers set to Jeroen Demeyer
  • Status changed from needs_review to positive_review

comment:3 Changed 11 months ago by vbraun

  • Status changed from positive_review to needs_work

See patchbot

comment:4 Changed 11 months ago by embray

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 11 months ago by git

  • Commit changed from bec801e3b085e866a88906561c912d6b03ef52a3 to db22bc7951f4a054166b99f5cb8f74af587f178f

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

db22bc7add an optional 'seed' argument to randstate.python_random

comment:6 Changed 11 months ago by embray

  • 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 11 months ago by chapoton

  • 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 10 months ago by vbraun

  • 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
Note: See TracTickets for help on using tickets.