#4412: [with patches, needs work] extend the local information function for
elliptic curves over number fields
---------------------------------------+------------------------------------
Reporter: cremona | Owner: was
Type: defect | Status: new
Priority: minor | Milestone: sage-3.2.2
Component: number theory | Resolution:
Keywords: elliptic curve local data |
---------------------------------------+------------------------------------
Changes (by was):
* summary: [with patches, needs review] extend the local information
function for elliptic curves over number fields
=> [with patches, needs work] extend the local
information function for elliptic curves over
number fields
Comment:
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)
}}}
2. 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
}}}
3. Giving a meaningless algorithm option should raise a ValueError:
{{{
sage: EllipticCurveLocalData(E,2, algorithm='foo bar')
}}}
4. This line (line 240)
{{{
if not cp==4:
}}}
looks silly. How about "if cp != 4:"?
5. 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.
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/4412#comment:4>
Sage <http://sagemath.org/>
Sage - Open Source Mathematical Software: Building the Car Instead of
Reinventing the Wheel
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"sage-trac" group.
To post to this group, send email to [email protected]
To unsubscribe from this group, send email to [EMAIL PROTECTED]
For more options, visit this group at
http://groups.google.com/group/sage-trac?hl=en
-~----------~----~----~----~------~----~------~--~---
