Ticket #4275 (closed enhancement: fixed)

Opened 2 months ago

Last modified 2 months ago

[with new patch, with positive review] improved doctest for elliptic curves (part 2)

Reported by: zimmerma Assigned to: was
Priority: minor Milestone: sage-3.2
Component: algebraic geometry Keywords:
Cc: cremona

Description

This patch adds missing doctests to ell_modular_symbols.py, ell_number_field.py and ell_padic_field.py. The conversion to Pari in ell_padic_field.py was broken, and still fails in some cases (see example in file), but I don't know if this is a bug in Pari, or an invalid input.

I also removed some unused functions in ell_modular_symbols.py, it would be good to check they are not needed elsewhere.

Note that some internal functions could not be tested, thus the coverage is not 100%. 

bash-3.00$ sage -coverage ell_modular_symbols.py ell_number_field.py ell_padic_field.py
----------------------------------------------------------------------
ell_modular_symbols.py
SCORE ell_modular_symbols.py: 100% (6 of 6)
----------------------------------------------------------------------

----------------------------------------------------------------------
ell_number_field.py
SCORE ell_number_field.py: 90% (19 of 21)

Missing documentation:
         * _proot(x, e):
         * _pquadroots (a, b, c):

----------------------------------------------------------------------

----------------------------------------------------------------------
ell_padic_field.py
SCORE ell_padic_field.py: 80% (4 of 5)

Missing documentation:
         * _frob(P):

Attachments

10554.patch (10.9 kB) - added by zimmerma on 10/13/2008 08:59:17 AM.
10556.patch (1.6 kB) - added by zimmerma on 10/14/2008 03:40:47 AM.
trac_4275_part3.patch (2.0 kB) - added by zimmerma on 10/14/2008 12:02:50 PM.
sage-trac4275.patch (7.1 kB) - added by cremona on 10/18/2008 01:54:14 AM.
Replaces the three earlier patches

Change History

10/13/2008 08:59:17 AM changed by zimmerma

  • attachment 10554.patch added.

10/13/2008 09:53:19 AM changed by cremona

Paul, can you say which version (exactly) your patch should apply to? I failed to apply it to 3.1.3.alpha3, and suspect that some of the changes I made between 3.1.2 and that one conflict with yours. (For example, I thought I had already converted some "internal functions" to lambda functions!) It is a habit of mine that when I work on a file I try to improve its doctest coverage at the same time, so this is not so surprising! John

10/13/2008 12:40:25 PM changed by zimmerma

John, my patch should apply to 3.1.2 (I always use the latest release). In the meantime, I have worked on ell_point.py, I hope this will be no duplicate work, I will submit it soon.

10/14/2008 03:40:47 AM changed by zimmerma

  • attachment 10556.patch added.

10/14/2008 03:42:34 AM changed by zimmerma

The second patch (to be applied after the first one) adds some documentation and one test for the Pari conversion (thanks Karim Belabas).

10/14/2008 12:02:50 PM changed by zimmerma

  • attachment trac_4275_part3.patch added.

10/14/2008 12:05:30 PM changed by zimmerma

  • summary changed from [with patch, needs review] improved doctest for elliptic curves (part 2) to [with patches, needs review] improved doctest for elliptic curves (part 2).

The 3rd patch (to be applied after the first 2) re-enable two methods that were unused in ell_modular_symbols.py, but one was used elsewhere, and the other one might be useful.

10/18/2008 01:54:14 AM changed by cremona

  • attachment sage-trac4275.patch added.

Replaces the three earlier patches

10/18/2008 01:55:38 AM changed by cremona

  • summary changed from [with patches, needs review] improved doctest for elliptic curves (part 2) to [with new patch, with positive review] improved doctest for elliptic curves (part 2).

The new patch sage-trac4275.patch merges the three previous ones and rebases them on 3.1.4. Essentially the same as Paul's 3. I am happy with these being merged.

10/18/2008 02:26:40 AM changed by zimmerma

  • cc set to cremona.

Thanks John. A quick summary of my (unfinished) doctest work on elliptic curves:

#4271 -> merged in 3.1.3

#4275 (this one): needs review

#4277 positive review, ready to merge

#4281 needs review

#4287 needs review + fix loads/dump problem

I put you in cc to be sure you get this. Feel free to remove yourself.

10/18/2008 02:29:20 AM changed by zimmerma

Sorry, one should read #4275 (this one): positive review, ready to merge instead of what I wrote above.

10/18/2008 12:48:29 PM changed by mabshoff

  • status changed from new to closed.
  • resolution set to fixed.

Merged in Sage 3.2.alpha0