Sage: Ticket #4981: [with patch, positive review] clean up polynomial_ring.py
https://trac.sagemath.org/ticket/4981
<p>
The way element classes are chosen in <code>sage/rings/polynomial/polynomial_ring.py</code> goes very much against object oriented design, and is basically ugly. :)
</p>
<p>
Attached patch tries to clean up this file, moves the decision of element classes to the immediate parents, adds some tests, and unifies the <code>__call__</code> methods. This also makes it much easier to add support for specialized polynomial classes.
</p>
en-usSagehttps://trac.sagemath.org/chrome/site/logo_sagemath_trac.png
https://trac.sagemath.org/ticket/4981
Trac 1.2John CremonaSun, 18 Jan 2009 17:58:30 GMTsummary changed
https://trac.sagemath.org/ticket/4981#comment:1
https://trac.sagemath.org/ticket/4981#comment:1
<ul>
<li><strong>summary</strong>
changed from <em>[with patch, needs review] clean up polynomial_ring.py</em> to <em>[with patch, with positive review] clean up polynomial_ring.py</em>
</li>
</ul>
<p>
Review: Patch applies cleanly to 3.2.3. All looks very sensible to me, and I trust burcin to know what he is doing, though I cannot say that I followed through all the logic. All doctests in rings/polynomial pass, so I think that this is good to go.
</p>
TicketMichael AbshoffMon, 19 Jan 2009 03:28:43 GMTsummary changed
https://trac.sagemath.org/ticket/4981#comment:2
https://trac.sagemath.org/ticket/4981#comment:2
<ul>
<li><strong>summary</strong>
changed from <em>[with patch, with positive review] clean up polynomial_ring.py</em> to <em>[with patch, needs work] clean up polynomial_ring.py</em>
</li>
</ul>
<p>
This patch causes the following trivial to fix doctest failure:
</p>
<pre class="wiki">mabshoff@geom:/scratch/mabshoff/sage-3.3.alpha0$ ./sage -t -long devel/sage/sage/calculus/calculus.py
sage -t -long "devel/sage/sage/calculus/calculus.py"
**********************************************************************
File "/scratch/mabshoff/sage-3.3.alpha0/devel/sage/sage/calculus/calculus.py", line 1912:
sage: type(a)
Expected:
<type 'sage.rings.polynomial.polynomial_element.Polynomial_generic_dense'>
Got:
<class 'sage.rings.polynomial.polynomial_element_generic.Polynomial_generic_dense_field'>
**********************************************************************
</pre><p>
Unfortunately the following test
</p>
<pre class="wiki">Trying:
Integer(2)*P + Integer(2)*Q # indirect doctest###line 208:_sage_ >>> 2*P + 2*Q # indirect doctest
Expecting:
(x^2 - 2*x + 1, y - 3/2*a*x + 1/2*a)
</pre><p>
in sage/schemes/hyperelliptic_curves/jacobian_morphism.py seems to loop forever - at least I killed it after it used 22 minutes of CPU time on the new sage.math.
</p>
<p>
Cheers,
</p>
<p>
Michael
</p>
TicketBurcin ErocalWed, 21 Jan 2009 08:15:53 GMTsummary changed
https://trac.sagemath.org/ticket/4981#comment:3
https://trac.sagemath.org/ticket/4981#comment:3
<ul>
<li><strong>summary</strong>
changed from <em>[with patch, needs work] clean up polynomial_ring.py</em> to <em>[with patch, needs review] clean up polynomial_ring.py</em>
</li>
</ul>
<p>
A new patch which fixes the doctests is attached.
</p>
TicketncalexanWed, 21 Jan 2009 19:04:20 GMTsummary changed
https://trac.sagemath.org/ticket/4981#comment:4
https://trac.sagemath.org/ticket/4981#comment:4
<ul>
<li><strong>summary</strong>
changed from <em>[with patch, needs review] clean up polynomial_ring.py</em> to <em>[with patch, with positive review] clean up polynomial_ring.py</em>
</li>
</ul>
<p>
Code wise: this looks great! I heartily agree with the sentiment and implementation.
</p>
<p>
Testing wise: I tested this on sage.math and think that it works.
</p>
TicketMichael AbshoffFri, 23 Jan 2009 02:41:08 GMTsummary changed
https://trac.sagemath.org/ticket/4981#comment:5
https://trac.sagemath.org/ticket/4981#comment:5
<ul>
<li><strong>summary</strong>
changed from <em>[with patch, with positive review] clean up polynomial_ring.py</em> to <em>[with patch, with positive review, needs rebase] clean up polynomial_ring.py</em>
</li>
</ul>
<p>
Unfortunately this has been broken due to other merges, probably <a class="closed ticket" href="https://trac.sagemath.org/ticket/4965" title="#4965: defect: [with patch, positive review] Z/nZ[x] via FLINT's zmod_poly (closed: fixed)">#4965</a>:
</p>
<pre class="wiki">mabshoff@geom:/scratch/mabshoff/sage-3.3.alpha1/devel/sage$ patch -p1 < trac_4981_polynomial_ring.patch
patching file sage/calculus/calculus.py
patching file sage/misc/classgraph.py
patching file sage/rings/polynomial/polynomial_ring.py
Hunk #2 succeeded at 86 with fuzz 2 (offset 2 lines).
Hunk #3 succeeded at 102 (offset 2 lines).
Hunk #4 succeeded at 111 (offset 2 lines).
Hunk #5 succeeded at 156 (offset 2 lines).
Hunk #6 succeeded at 167 (offset 2 lines).
Hunk #7 succeeded at 231 (offset 2 lines).
Hunk #8 succeeded at 277 (offset 2 lines).
Hunk #9 succeeded at 490 (offset 2 lines).
Hunk #10 FAILED at 504.
Hunk #11 succeeded at 968 (offset 2 lines).
Hunk #12 succeeded at 983 (offset 2 lines).
Hunk #13 FAILED at 1125.
2 out of 13 hunks FAILED -- saving rejects to file sage/rings/polynomial/polynomial_ring.py.rej
patching file sage/rings/polynomial/polynomial_template.pxi
Hunk #1 FAILED at 85.
1 out of 1 hunk FAILED -- saving rejects to file sage/rings/polynomial/polynomial_template.pxi.rej
</pre><p>
Please rebase for 3.3.alpha1.
</p>
<p>
Cheers,
</p>
<p>
Michael
</p>
TicketMichael AbshoffSat, 24 Jan 2009 17:33:27 GMTsummary changed
https://trac.sagemath.org/ticket/4981#comment:6
https://trac.sagemath.org/ticket/4981#comment:6
<ul>
<li><strong>summary</strong>
changed from <em>[with patch, with positive review, needs rebase] clean up polynomial_ring.py</em> to <em>[with patch, positive review] clean up polynomial_ring.py</em>
</li>
</ul>
<p>
I fixed a tiny doctesting issue in the second patch:
</p>
<pre class="wiki">sage -t -long "devel/sage/sage/rings/polynomial/polynomial_quotient_ring.py"
**********************************************************************
File "/scratch/mabshoff/sage-3.3.alpha2/devel/sage/sage/rings/polynomial/polynomial_quotient_ring.py", line 84:
sage: A.<y> = PolynomialRing(GF(2)); A
Expected:
Univariate Polynomial Ring in y over Finite Field of size 2
Got:
Univariate Polynomial Ring in y over Finite Field of size 2 (using NTL)
**********************************************************************
1 items had failures:
</pre><p>
Burcin explained how he fixed the hang, positive review.
</p>
<p>
Cheers,
</p>
<p>
Michael
</p>
TicketBurcin ErocalSat, 24 Jan 2009 17:36:47 GMTattachment set
https://trac.sagemath.org/ticket/4981
https://trac.sagemath.org/ticket/4981
<ul>
<li><strong>attachment</strong>
set to <em>polynomial_ring.patch</em>
</li>
</ul>
<p>
clean up polynomial_ring.py (take 4)
</p>
TicketMichael AbshoffSat, 24 Jan 2009 17:45:36 GMTstatus changed; resolution set
https://trac.sagemath.org/ticket/4981#comment:7
https://trac.sagemath.org/ticket/4981#comment:7
<ul>
<li><strong>status</strong>
changed from <em>new</em> to <em>closed</em>
</li>
<li><strong>resolution</strong>
set to <em>fixed</em>
</li>
</ul>
<p>
Merged polynomial_ring.py (take 4) in Sage 3.3.alpha2
</p>
Ticket