Opened 6 months ago

Closed 6 months 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) Commit: bf37ac8d4f9c86be74985b0e10824ec19b170ecf
Dependencies: Stopgaps:

Description

in very untested and neglected code

Change History (20)

comment:1 Changed 6 months ago by chapoton

  • Branch set to u/chapoton/27432
  • Cc cremona davidloeffler added
  • Commit set to 473ef25385de5048c69379574307b0f7d3e8b477
  • Status changed from new to needs_review

New commits:

473ef25py3: fix last doctest in ell_rational_field (in badly neglected code)

comment:2 Changed 6 months ago by chapoton

  • Cc tscrim added

green bot, please review

comment:3 Changed 6 months ago by tscrim

  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

LGTM.

comment:4 Changed 6 months ago by 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.)

Last edited 6 months ago by cremona (previous) (diff)

comment:5 Changed 6 months ago by chapoton

  • Status changed from positive_review to needs_work

comment:6 Changed 6 months ago by git

  • Commit changed from 473ef25385de5048c69379574307b0f7d3e8b477 to cf5e2436786c703b30a250d73d4905875e9607e1

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

cf5e243trac 27432 much better doctests

comment:7 Changed 6 months ago by chapoton

  • 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 6 months ago by tscrim

  • 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 6 months ago by tscrim

  • 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 6 months ago by 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 * 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 6 months ago by 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.

comment:12 Changed 6 months ago by 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...

comment:13 follow-up: Changed 6 months ago by chapoton

This method is just called nowhere at all.

comment:14 in reply to: ↑ 13 Changed 6 months ago by 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.

comment:15 Changed 6 months ago by vbraun

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

  • Commit changed from cf5e2436786c703b30a250d73d4905875e9607e1 to bf37ac8d4f9c86be74985b0e10824ec19b170ecf

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

1b05aefMerge branch 'u/chapoton/27432' in 8.7.b7
bf37ac8trac 27432 adding abs tol for numerical noise

comment:17 Changed 6 months ago by chapoton

grumble... I have added abs tol

comment:18 Changed 6 months ago by chapoton

  • Status changed from needs_work to needs_review

comment:19 Changed 6 months ago by tscrim

  • Status changed from needs_review to positive_review

comment:20 Changed 6 months ago by vbraun

  • Branch changed from u/chapoton/27432 to bf37ac8d4f9c86be74985b0e10824ec19b170ecf
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.