Sage: Ticket #5890: [with patch, woth positive review] clean up schemes/elliptic_curves/ell_generic.py
https://trac.sagemath.org/ticket/5890
<p>
As noted at <a class="closed ticket" href="https://trac.sagemath.org/ticket/5765" title="defect: [with patch, with positive review] improve doctest coverage for ... (closed: fixed)">#5765</a>, <code>ell_generic.py</code> has some functions that do not make sense over a general ring and should rather be moved down to <code>ell_field.py</code> or one of its descendants.
</p>
<p>
Note also William's comment from <a class="closed ticket" href="https://trac.sagemath.org/ticket/5765" title="defect: [with patch, with positive review] improve doctest coverage for ... (closed: fixed)">#5765</a>: I think it would be nice to be able to implement the elliptic curve factorization method (ECM) without having to use this hack:
</p>
<pre class="wiki">R = Zmod(N)
R.is_field = lambda: True
E = EllipticCurve(R, [-1,0])
</pre>en-usSagehttps://trac.sagemath.org/chrome/site/logo_sagemath_trac.png
https://trac.sagemath.org/ticket/5890
Trac 1.1.6AlexGhitzaSat, 25 Apr 2009 05:07:29 GMTcc set
https://trac.sagemath.org/ticket/5890#comment:1
https://trac.sagemath.org/ticket/5890#comment:1
<ul>
<li><strong>cc</strong>
<em>was</em> added
</li>
</ul>
<p>
For the record, to understand William's comment have a look at line 572 of <code>tests/book_stein_ent.py</code>, where he implements ECM and uses this hack.
</p>
<p>
However, his function works for me in 3.4.1, so I think it's already been fixed.
</p>
TicketAlexGhitzaSat, 25 Apr 2009 05:08:18 GMT
https://trac.sagemath.org/ticket/5890#comment:2
https://trac.sagemath.org/ticket/5890#comment:2
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/5890#comment:1" title="Comment 1">AlexGhitza</a>:
</p>
<blockquote class="citation">
<p>
For the record, to understand William's comment have a look at line 572 of <code>tests/book_stein_ent.py</code>, where he implements ECM and uses this hack.
</p>
<p>
However, his function works for me in 3.4.1, so I think it's already been fixed.
</p>
</blockquote>
<blockquote>
<p>
I mean of course that it works for me in 3.4.1 without the hackish line that fools Sage into thinking that R is a field.
</p>
</blockquote>
TicketAlexGhitzaSat, 25 Apr 2009 05:15:46 GMTstatus, owner, summary changed
https://trac.sagemath.org/ticket/5890#comment:3
https://trac.sagemath.org/ticket/5890#comment:3
<ul>
<li><strong>status</strong>
changed from <em>new</em> to <em>assigned</em>
</li>
<li><strong>owner</strong>
changed from <em>was</em> to <em>AlexGhitza</em>
</li>
<li><strong>summary</strong>
changed from <em>clean up schemes/elliptic_curves/ell_generic.py</em> to <em>[with patch, needs review] clean up schemes/elliptic_curves/ell_generic.py</em>
</li>
</ul>
<p>
The attached patch does some cleaning up, and it depends on <a class="closed ticket" href="https://trac.sagemath.org/ticket/5765" title="defect: [with patch, with positive review] improve doctest coverage for ... (closed: fixed)">#5765</a>.
</p>
<p>
It moves all the twists-related methods from <code>ell_generic.py</code> to <code>ell_field.py</code>, as well as the alias <code>base_field = base_ring</code>.
</p>
<p>
It also makes <code>change_ring</code> into an alias for <code>base_extend</code>, since the two have the exact same functionality and equivalent code.
</p>
<p>
Lastly, the standalone function <code>Hasse_bounds</code> is moved from <code>ell_generic.py</code> to <code>schemes/plane_curves/projective_curve.py</code>, which is a more natural place for it.
</p>
<p>
There are more things to do, but they are issues with the generic code for curves rather than the code for elliptic curves, so I think they should be fixed in <code>schemes/plane_curves</code> instead. I'll be looking into that soon.
</p>
TicketAlexGhitzaSat, 25 Apr 2009 05:16:24 GMTattachment set
https://trac.sagemath.org/ticket/5890
https://trac.sagemath.org/ticket/5890
<ul>
<li><strong>attachment</strong>
set to <em>trac_5890.patch</em>
</li>
</ul>
<p>
depends on the latest patch at <a class="closed ticket" href="https://trac.sagemath.org/ticket/5765" title="defect: [with patch, with positive review] improve doctest coverage for ... (closed: fixed)">#5765</a>
</p>
TicketAlexGhitzaSat, 25 Apr 2009 05:26:32 GMTcc changed
https://trac.sagemath.org/ticket/5890#comment:4
https://trac.sagemath.org/ticket/5890#comment:4
<ul>
<li><strong>cc</strong>
<em>cremona</em> added
</li>
</ul>
TicketcremonaWed, 29 Apr 2009 11:39:46 GMTsummary changed
https://trac.sagemath.org/ticket/5890#comment:5
https://trac.sagemath.org/ticket/5890#comment:5
<ul>
<li><strong>summary</strong>
changed from <em>[with patch, needs review] clean up schemes/elliptic_curves/ell_generic.py</em> to <em>[with patch, woth positive review] clean up schemes/elliptic_curves/ell_generic.py</em>
</li>
</ul>
<p>
Review: first of all, this is just moving code around, all perfectly sensible (lots of stuff moved down from ell_generic to ell_field, and Hasse_bound function moved off to plane curves (where I should have put it in the first place).
</p>
<p>
I applied first 12097.patch (from <a class="closed ticket" href="https://trac.sagemath.org/ticket/5919" title="defect: [with patch, positive review] bug in conversion of polys over GF(2,e) ... (closed: fixed)">#5919</a>) then trac_5765-rebased.patch (from <a class="closed ticket" href="https://trac.sagemath.org/ticket/5765" title="defect: [with patch, with positive review] improve doctest coverage for ... (closed: fixed)">#5765</a>) and then trac_5890.patch (from here), all successfully.
</p>
<p>
Doctests in schemes/plane_curves and schemes/elliptic_curves pass. I will give this a positive review, despite the following, which will make it harder to do EC factoring (but the fault lies not in the patch here, rather in moving the test for a point lying on a curve which is now more sophisticated to harder to fool.... but that is not for this ticket to sort out.
</p>
<p>
The example
</p>
<pre class="wiki">N = 1001
R = Zmod(N)
R.is_field = lambda: True
E = EllipticCurve(R, [-1,0])
</pre><p>
works but you cannot construct a point in the curve (e.g. E(0,0)) since this happens:
</p>
<pre class="wiki">sage: P = E(0,0)
---------------------------------------------------------------------------
NotImplementedError Traceback (most recent call last)
/home/masgaj/.sage/temp/host_56_150/32116/_home_masgaj__sage_init_sage_0.py in <module>()
/local/jec/sage-3.4.2.alpha0/local/lib/python2.5/site-packages/sage/schemes/elliptic_curves/ell_generic.pyc in __call__(self, *args, **kwds)
609 args = tuple(args[0])
610
--> 611 return plane_curve.ProjectiveCurve_generic.__call__(self, *args, **kwds)
612
613 def _reduce_point(self, R, p):
/local/jec/sage-3.4.2.alpha0/local/lib/python2.5/site-packages/sage/schemes/generic/scheme.pyc in __call__(self, *args)
196 else:
197 return self.point(S)
--> 198 return self.point(args)
199
200 def point_homset(self, S = None):
/local/jec/sage-3.4.2.alpha0/local/lib/python2.5/site-packages/sage/schemes/generic/scheme.pyc in point(self, v, check)
230
231 def point(self, v, check=True):
--> 232 return self._point_class(self, v, check=check)
233
234 def _point_class(self):
/local/jec/sage-3.4.2.alpha0/local/lib/python2.5/site-packages/sage/schemes/generic/morphism.pyc in __init__(self, X, v, check)
415 """
416 def __init__(self, X, v, check=True):
--> 417 raise NotImplementedError
418
419
NotImplementedError:
</pre>
TicketmabshoffThu, 30 Apr 2009 00:54:18 GMTstatus, milestone changed; resolution set
https://trac.sagemath.org/ticket/5890#comment:6
https://trac.sagemath.org/ticket/5890#comment:6
<ul>
<li><strong>status</strong>
changed from <em>assigned</em> to <em>closed</em>
</li>
<li><strong>resolution</strong>
set to <em>fixed</em>
</li>
<li><strong>milestone</strong>
changed from <em>sage-4.0</em> to <em>sage-3.4.2</em>
</li>
</ul>
<p>
Merged in Sage 3.4.2.rc0.
</p>
<p>
Cheers,
</p>
<p>
Michael
</p>
Ticket