Opened 14 years ago

Closed 14 years ago

Last modified 14 years ago

#4412 closed defect (fixed)

[with patches, positive review] extend the local information function for elliptic curves over number fields

Reported by: cremona Owned by: was
Priority: minor Milestone: sage-3.2.1
Component: number theory Keywords: elliptic curve local data
Cc: AlexGhitza Merged in:
Authors: Reviewers:
Report Upstream: Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description

This is essentially a continuation of #3897. I have added functionality to ell_local_data.py so that for elliptic curves over number fields (and over Q) you can (1) ask about additive vs. split vs. non-split multiplicative reduction at a prime; (2) Ask for the Tamagawa index (which is not always equal to the T. number) and also (3) added some better documentation to the kodaira_symbol code.

The motivation is that this is used i computing p-adic elliptic logs which in turn in used in the S-integral points code which is coming along nicely. But this stuff is independent of that so I thought it could be posted separately.

The patch should apply to 3.2.alpha1.

Attachments (4)

sage-localdata.patch (42.7 KB) - added by cremona 14 years ago.
4412-typo-localdata.patch (18.5 KB) - added by mvngu 14 years ago.
fix typos found after applying cremona's patch
trac_sage-4412_typos-rebased.patch (17.6 KB) - added by was 14 years ago.
trac_sage-4412_post-review.patch (7.3 KB) - added by cremona 14 years ago.

Download all attachments as: .zip

Change History (13)

Changed 14 years ago by cremona

Changed 14 years ago by mvngu

fix typos found after applying cremona's patch

comment:1 follow-up: Changed 14 years ago by mvngu

  • Summary changed from [with patch, needs review] extend the local information function for elliptic curves over number fields to [with patches, needs review] extend the local information function for elliptic curves over number fields

The patch 4412-typo-localdata.patch was produced under sage-3.1.4. It fixes various typos that were found after applying cremona's patch sage-localdata.patch. That is, 4412-typo-localdata.patch should be applied on top of sage-localdata.patch.

comment:2 in reply to: ↑ 1 Changed 14 years ago by cremona

Replying to mvngu:

The patch 4412-typo-localdata.patch was produced under sage-3.1.4. It fixes various typos that were found after applying cremona's patch sage-localdata.patch. That is, 4412-typo-localdata.patch should be applied on top of sage-localdata.patch.

Many thanks! I was relieved to see that most of those typos are pre-existing ones and not new ones introduced by me. In the place where you give two alternatives I prefer the first one (and that one is my fault). John

comment:3 Changed 14 years ago by was

REFEREEing:

Applies and all elliptic_curve tests pass. I had to slightly rebase the typo fix patch, and fix the "which do you mean" issue.

All tests passed!
Total time for all tests: 67.1 seconds
was@sage:~/build/sage-3.2.1.alpha1$ 

I've attached the rebased patch.

Changed 14 years ago by was

comment:4 Changed 14 years ago by was

  • Summary changed from [with patches, needs review] extend the local information function for elliptic curves over number fields to [with patches, needs work] extend the local information function for elliptic curves over number fields

REFEREE REPORT:

This is an extremely good patch, with about a 10:1 ratio of documentation to code, and it really really needs to get in. Here are a few minor issues that need to get fixed. When they are all fixed, I'll give this a positive review.

  1. Please add a doctest to illustrate the algorithm= option to EllipticCurveLocalData?, since all the doctests look like this, and none illustrate that new parameter.
    EllipticCurveLocalData(E,7)
    
  1. Once you do 1, you'll find it doesn't work, at least in the only example I tried:
    sage: E = EllipticCurve('14a1') 
    sage: from sage.schemes.elliptic_curves.ell_local_data import EllipticCurveLocalData 
    sage: EllipticCurveLocalData(E,2, algorithm='generic')
    ---------------------------------------------------------------------------
    TypeError                                 Traceback (most recent call last)
    
    /home/was/build/sage-3.2.1.alpha1/<ipython console> in <module>()
    
    /home/was/build/sage-3.2.1.alpha1/local/lib/python2.5/site-packages/sage/schemes/elliptic_curves/ell_local_data.pyc in __init__(self, E, P, proof, algorithm)
        110             self._Emin, ch, self._val_disc, self._fp, self._KS, self._cp, self._split = self._tate(proof)
        111             if self._fp>0:
    --> 112                 if self._Emin.c4().valuation(p)>0:
        113                     self._reduction_type = 0
        114                 elif self._split:    
    
    /home/was/build/sage-3.2.1.alpha1/local/lib/python2.5/site-packages/sage/rings/rational.so in sage.rings.rational.Rational.valuation (sage/rings/rational.c:6338)()
    
    /home/was/build/sage-3.2.1.alpha1/local/lib/python2.5/site-packages/sage/rings/integer.so in sage.rings.integer.Integer.valuation (sage/rings/integer.c:14944)()
    
    /home/was/build/sage-3.2.1.alpha1/local/lib/python2.5/site-packages/sage/rings/integer.so in sage.rings.integer.Integer.__init__ (sage/rings/integer.c:6054)()
    
    TypeError: unable to coerce <class 'sage.rings.ideal.Ideal_pid'> to an integer
    
  1. Giving a meaningless algorithm option should raise a ValueError?:
    sage: EllipticCurveLocalData(E,2, algorithm='foo bar')
    
  1. This line (line 240)
    if not cp==4: 
    

looks silly. How about "if cp != 4:"?

  1. For consistency in your docstrings in the assignments could you put spaces

around =? For example, you have

        476	            sage: K.<a>=NumberField(x^3-2) 
 	477	            sage: P17a, P17b = [P for P,e in K.factor(17)] 
 	478	            sage: E = EllipticCurve([0,0,0,0,2*a+1]) 

so sometimes there is space (which I really like!) and sometimes there isn't.

Changed 14 years ago by cremona

comment:5 Changed 14 years ago by cremona

Thanks for the detailed review. The latest patch addresses all of those:

  1. Extra tests added
  2. Fixed (really a logic error)
  3. A ValueError? is now raised (see extra doctest)
  4. Changed
  5. Changed (I agree with the convention but some always slip through!)

Tested on 3.2.1.rc0, all tests in elliptic_curves/ pass.

comment:6 Changed 14 years ago by cremona

  • Summary changed from [with patches, needs work] extend the local information function for elliptic curves over number fields to [with patches, needs review (again)] extend the local information function for elliptic curves over number fields

comment:7 Changed 14 years ago by was

  • Summary changed from [with patches, needs review (again)] extend the local information function for elliptic curves over number fields to [with patches, positive review] extend the local information function for elliptic curves over number fields

comment:8 Changed 14 years ago by mabshoff

  • Resolution set to fixed
  • Status changed from new to closed

Merged sage-localdata.patch, trac_sage-4412_typos-rebased.patch and trac_sage-4412_post-review.patch in Sage 3.2.1.rc1

Cheers,

Michael

comment:9 Changed 14 years ago by mabshoff

  • Milestone changed from sage-3.2.2 to sage-3.2.1
Note: See TracTickets for help on using tickets.