py3: fix last doctest in ell_rational_field

[chapoton]

in very untested and neglected code

[chapoton]

New commits:

​473ef25

py3: fix last doctest in ell_rational_field (in badly neglected code)

[chapoton]

green bot, please review

[tscrim]

LGTM.

[cremona]

No! The examples for eval_modular_form() are stupid (before and after). The badly-named prec parameter is not a floating point precision, but should be a positive integer since (looking at the code) it says how many terms of the q-expansion to use. For the example a value of 100 would be reasonable. No wonder the "values" at the three points are 0, since 0 terms of the series have been added!

sage: E = EllipticCurve('37a1')
sage: E.eval_modular_form([1.5+I,2.0+I,2.5+I],100)
[-0.0018743978548152085771342944989052703431,
 0.0018604485340371083710285594393397945456,
 -0.0018743978548152085771342944989052703431]

is reasonable. Even 10 gives the same result. (Convergence depends on the size of the imaginary part, and these points have imaginary part 1.)

[git]

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

​cf5e243

trac 27432 much better doctests

[chapoton]

now with much better doctests (thx John)

let us hope that there is no numerical noise.

[tscrim]

Then let's send it off to the buildbots to see if they come back with soemthing on 32-bit (at least 3 patchbots and my machine did not have any noise, but I do not have access to 32-bit to test).

[tscrim]

John, I have added you as a reviewer for (the very helpful) comment:4.

[cremona]

I would have replaced the tests myself but did not have time yesterday. I think we should. Other suggestions: possibly change the name of the "prec" parameter, or even replace it by a more usual floating precision parameter and work out how many terms to sum from that. It's simply the sum of a_n * q^{n over however many terms, where the a_n are obtained from the pari function, and q=exp(2*pi*i*z). Since Im(z)>0 we have |q|<1, and standard estimates give |a_n|=soft-O(n}(1/2)) (there is a log factor). So convergence is fast unless Im(z) is very small, while computing a_n is cheap since pari does that efficiently. An easy but weak estimate gives |a_n|<=n.

[chapoton]

Tests have been replaced by your proposal, and the name prec changed to order.

We should keep other enhancement for other tickets. The main aim of the present ticket is to fix one file for python3.

[cremona]

OK thanks -- the new doctests are much better. I hope that nowhere else in the library this method is called with the old 'prec' parameter named explicitly...

[chapoton]

This method is just called nowhere at all.

[cremona]

Replying to chapoton:

This method is just called nowhere at all.

Thanks for checking. I think I can see why -- there is a better version in modular_parametrization.py in the method map_to_complex_numbers() which takes a bit precision 'prec' parameter and works out the number of terms (quoting my code in eclib for that!).

Compare

sage: E = EllipticCurve('37a1')
sage: E.eval_modular_form(1+I, 100)
[0.0018604485340371083710285594393397945456]

with

sage: E.modular_parametrization().map_to_complex_numbers(1+I,100)
0.0018639488830113800896666432794 - 6.3094713246691959525967229233e-34*I

where the parameter 100 in the last call is bit precision. It would be reasonable to deprecate the function we have been talking about and/or replace its implementation with a call to the modular_parametrization one.

[vbraun]

File "src/sage/schemes/elliptic_curves/ell_rational_field.py", line 5381, in sage.schemes.elliptic_curves.ell_rational_field.EllipticCurve_rational_field.eval_modular_form
Failed example:
    E.eval_modular_form([1.5+I,2.0+I,2.5+I],100)
Expected:
    [-0.0018743978548152085771342944989052703431,
     0.0018604485340371083710285594393397945456,
    -0.0018743978548152085771342944989052703431]
Got:
    [-0.001874397854815208577134294499,
     0.001860448534037108371028559439,
     -0.001874397854815208577134294499]
**********************************************************************
1 item had failures:
   1 of   4 in sage.schemes.elliptic_curves.ell_rational_field.EllipticCurve_rational_field.eval_modular_form
    [847 tests, 1 failure, 222.68 s]

[git]

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

​1b05aef	Merge branch 'u/chapoton/27432' in 8.7.b7
​bf37ac8	trac 27432 adding abs tol for numerical noise

[chapoton]

grumble... I have added abs tol