Opened 3 years ago
Closed 3 years ago
#27432 closed enhancement (fixed)
py3: fix last doctest in ell_rational_field
Reported by:  chapoton  Owned by:  

Priority:  major  Milestone:  sage8.7 
Component:  python3  Keywords:  
Cc:  cremona, davidloeffler, tscrim  Merged in:  
Authors:  Frédéric Chapoton  Reviewers:  Travis Scrimshaw, John Cremona 
Report Upstream:  N/A  Work issues:  
Branch:  bf37ac8 (Commits, GitHub, GitLab)  Commit:  bf37ac8d4f9c86be74985b0e10824ec19b170ecf 
Dependencies:  Stopgaps: 
Description
in very untested and neglected code
Change History (20)
comment:1 Changed 3 years ago by
 Branch set to u/chapoton/27432
 Cc cremona davidloeffler added
 Commit set to 473ef25385de5048c69379574307b0f7d3e8b477
 Status changed from new to needs_review
comment:3 Changed 3 years ago by
 Reviewers set to Travis Scrimshaw
 Status changed from needs_review to positive_review
LGTM.
comment:4 Changed 3 years ago by
No! The examples for eval_modular_form() are stupid (before and after). The badlynamed 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 qexpansion 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.)
comment:5 Changed 3 years ago by
 Status changed from positive_review to needs_work
comment:6 Changed 3 years ago by
 Commit changed from 473ef25385de5048c69379574307b0f7d3e8b477 to cf5e2436786c703b30a250d73d4905875e9607e1
Branch pushed to git repo; I updated commit sha1. New commits:
cf5e243  trac 27432 much better doctests

comment:7 Changed 3 years ago by
 Status changed from needs_work to needs_review
now with much better doctests (thx John)
let us hope that there is no numerical noise.
comment:8 Changed 3 years ago by
 Status changed from needs_review to positive_review
Then let's send it off to the buildbots to see if they come back with soemthing on 32bit (at least 3 patchbots and my machine did not have any noise, but I do not have access to 32bit to test).
comment:9 Changed 3 years ago by
 Reviewers changed from Travis Scrimshaw to Travis Scrimshaw, John Cremona
John, I have added you as a reviewer for (the very helpful) comment:4.
comment:10 Changed 3 years ago by
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=softO(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.
comment:11 Changed 3 years ago by
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.
comment:12 Changed 3 years ago by
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...
comment:13 followup: ↓ 14 Changed 3 years ago by
This method is just called nowhere at all.
comment:14 in reply to: ↑ 13 Changed 3 years ago by
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.3094713246691959525967229233e34*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.
comment:15 Changed 3 years ago by
 Status changed from positive_review to needs_work
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]
comment:16 Changed 3 years ago by
 Commit changed from cf5e2436786c703b30a250d73d4905875e9607e1 to bf37ac8d4f9c86be74985b0e10824ec19b170ecf
comment:17 Changed 3 years ago by
grumble... I have added abs tol
comment:18 Changed 3 years ago by
 Status changed from needs_work to needs_review
comment:19 Changed 3 years ago by
 Status changed from needs_review to positive_review
comment:20 Changed 3 years ago by
 Branch changed from u/chapoton/27432 to bf37ac8d4f9c86be74985b0e10824ec19b170ecf
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
py3: fix last doctest in ell_rational_field (in badly neglected code)