#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: |
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)
Change History (13)
Changed 14 years ago by
Changed 14 years ago by
comment:1 follow-up: ↓ 2 Changed 14 years ago by
- 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
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
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
comment:4 Changed 14 years ago by
- 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.
- 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)
- 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
- Giving a meaningless algorithm option should raise a ValueError?:
sage: EllipticCurveLocalData(E,2, algorithm='foo bar')
- This line (line 240)
if not cp==4:
looks silly. How about "if cp != 4:"?
- 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
comment:5 Changed 14 years ago by
Thanks for the detailed review. The latest patch addresses all of those:
- Extra tests added
- Fixed (really a logic error)
- A ValueError? is now raised (see extra doctest)
- Changed
- 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
- 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
- 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
- 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
- Milestone changed from sage-3.2.2 to sage-3.2.1
fix typos found after applying cremona's patch