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: | sage-8.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 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.)
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 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).
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 * qn 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.
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 follow-up: ↓ 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.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.
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)