Sage: Ticket #1440: [with patch, with positive review] Inconsistency in subs and substitute for univariate polynomials
https://trac.sagemath.org/ticket/1440
<p>
I very much agree that the problem below is a bug, which we should resolve.
</p>
<pre class="wiki">subs and substitute are not equivalent for single variable
polynomials,
though they are in the field of fractions or for polynomials in more
than
one variable:
----------------------------------------------------------------------
| SAGE Version 2.8.15, Release Date: 2007-12-03 |
| Type notebook() for the GUI, and license() for information. |
----------------------------------------------------------------------
sage: R.<x> = QQ[]
sage: f = x^3 + x - 3
sage: f.subs(x = 5)
127
sage: f.substitute(x = 5)
---------------------------------------------------------------------------
<type 'exceptions.ValueError'> Traceback (most recent call
last)
/Users/mafwc/<ipython console> in <module>()
/Users/mafwc/element.pyx in
sage.structure.element.Element.substitute()
/Users/mafwc/polynomial_element.pyx in
sage.rings.polynomial.polynomial_element.Polynomial.subs()
/Users/mafwc/polynomial_element.pyx in
sage.rings.polynomial.polynomial_element.Polynomial.__call__()
<type 'exceptions.ValueError'>: must not specify both a keyword and
positional argument
sage: g = f/(x - 1)
sage: [g.subs(x = 5), g.substitute(x = 5)]
[127/4, 127/4]
sage: R2.<y, z> = PolynomialRing(QQ, 2)
sage: h = y^3*z + 4*y*z^2 + y + 3*z - 7
sage: [h.subs(y = 5), h.substitute(y = 5)]
[20*z^2 + 128*z - 2, 20*z^2 + 128*z - 2]
[Mac OS X 10.4.11, 2 GHz Intel Core 2 Duo, 1 GB].
</pre>en-usSagehttps://trac.sagemath.org/chrome/site/logo_sagemath_trac.png
https://trac.sagemath.org/ticket/1440
Trac 1.1.6jbmohlerWed, 26 Dec 2007 14:48:19 GMT
https://trac.sagemath.org/ticket/1440#comment:1
https://trac.sagemath.org/ticket/1440#comment:1
<p>
This is somewhat tangential to this issue, but subs has been overridden in a few different classes and the overridden implementation is not quite as robust as (or duplicates code from) the implementation in the element base class. I think that the base implementation/architecture should be strengthened in ways that make these overrides unneeded. Of course, then the subs/substitute synonym should entirely be handled by the base class making it impossible for the inconsistency of the noted bug.
</p>
<p>
Note that because of the issues I mention in the first paragraph I highly suspect that this bug is not unique to univ-variate polys.
</p>
TicketbrouneSat, 10 May 2008 22:59:43 GMTowner, summary changed
https://trac.sagemath.org/ticket/1440#comment:2
https://trac.sagemath.org/ticket/1440#comment:2
<ul>
<li><strong>owner</strong>
changed from <em>somebody</em> to <em>broune</em>
</li>
<li><strong>summary</strong>
changed from <em>Inconsistency in subs and substitute for univariate polynomials</em> to <em>[with patch, needs review] Inconsistency in subs and substitute for univariate polynomials</em>
</li>
</ul>
TicketbrouneSat, 10 May 2008 22:59:49 GMTstatus changed
https://trac.sagemath.org/ticket/1440#comment:3
https://trac.sagemath.org/ticket/1440#comment:3
<ul>
<li><strong>status</strong>
changed from <em>new</em> to <em>assigned</em>
</li>
</ul>
TicketbrouneSat, 10 May 2008 23:00:32 GMTattachment set
https://trac.sagemath.org/ticket/1440
https://trac.sagemath.org/ticket/1440
<ul>
<li><strong>attachment</strong>
set to <em>redef_substi.changeset</em>
</li>
</ul>
TicketcremonaTue, 13 May 2008 21:56:50 GMTsummary changed
https://trac.sagemath.org/ticket/1440#comment:4
https://trac.sagemath.org/ticket/1440#comment:4
<ul>
<li><strong>summary</strong>
changed from <em>[with patch, needs review] Inconsistency in subs and substitute for univariate polynomials</em> to <em>[with patch, with partial review] Inconsistency in subs and substitute for univariate polynomials</em>
</li>
</ul>
<p>
Review and questions:
</p>
<p>
If this is just an alias would it not be simpler to just put
</p>
<pre class="wiki"> substitute = subs
</pre><p>
in the class definition instead of defining a second function which calls the first?
</p>
<p>
Secondly, for some reason the patch posted does not display in the normal way, but I don't know why.
</p>
<p>
I don't really know enough about the issues raised by jbmohler to judge this solution; but if this does solve the problem then the simple assignment I suggested would also!
</p>
TicketbrouneTue, 13 May 2008 22:33:10 GMT
https://trac.sagemath.org/ticket/1440#comment:5
https://trac.sagemath.org/ticket/1440#comment:5
<p>
Is it possible to attach a docstring if the function is written using an assignment? Otherwise you would get no docstring, or possibly the docstring for subs, upon calling help on subtitute.
</p>
TicketcremonaWed, 14 May 2008 08:14:52 GMT
https://trac.sagemath.org/ticket/1440#comment:6
https://trac.sagemath.org/ticket/1440#comment:6
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/1440#comment:5" title="Comment 5">broune</a>:
</p>
<blockquote class="citation">
<p>
Is it possible to attach a docstring if the function is written using an assignment? Otherwise you would get no docstring, or possibly the docstring for subs, upon calling help on subtitute.
</p>
</blockquote>
<p>
Not literally, but after the assignment they are the <span class="underline">same</span> function so the docstring for one is displayed for both. For example, <code>prime_factors</code> is a synonym for <code>prime_divisors</code>, and look:
</p>
<pre class="wiki">sage: n=123
sage: n.prime_factors?
Type: builtin_function_or_method
Base Class: <type 'builtin_function_or_method'>
String Form: <built-in method prime_divisors of sage.rings.integer.Integer object at 0xa124980>
Namespace: Interactive
Docstring:
The prime divisors of self, sorted in increasing order. If n
is negative, we do *not* include -1 among the prime divisors, since -1 is
not a prime number.
EXAMPLES:
sage: a = 1; a.prime_divisors()
[]
sage: a = 100; a.prime_divisors()
[2, 5]
sage: a = -100; a.prime_divisors()
[2, 5]
sage: a = 2004; a.prime_divisors()
[2, 3, 167]
</pre><p>
Although that looks slightly strange, I think it is ok, especially if the docstring mentions the synonym (which in this case it does not, which is my fault since I inserted the synonym while no-one was looking).
</p>
TicketbrouneWed, 14 May 2008 14:10:52 GMTattachment set
https://trac.sagemath.org/ticket/1440
https://trac.sagemath.org/ticket/1440
<ul>
<li><strong>attachment</strong>
set to <em>redef_substi2.patch</em>
</li>
</ul>
<p>
Replacement for previous patch
</p>
TicketbrouneWed, 14 May 2008 14:13:49 GMT
https://trac.sagemath.org/ticket/1440#comment:7
https://trac.sagemath.org/ticket/1440#comment:7
<p>
The new patch uses = to redefine substitute. I would add a doctest, but I don't know where it would go. It does make the code in the ticket work. Either version is fine by me.
</p>
TicketcremonaWed, 14 May 2008 21:39:16 GMTsummary changed
https://trac.sagemath.org/ticket/1440#comment:8
https://trac.sagemath.org/ticket/1440#comment:8
<ul>
<li><strong>summary</strong>
changed from <em>[with patch, with partial review] Inconsistency in subs and substitute for univariate polynomials</em> to <em>[with patch, with positive review] Inconsistency in subs and substitute for univariate polynomials</em>
</li>
</ul>
<p>
I can see no reason not to apply this one-liner (redef_substi2.patch), even if there are other related issues which the patch does not resolve.
</p>
TicketmalbWed, 04 Jun 2008 20:38:04 GMT
https://trac.sagemath.org/ticket/1440#comment:9
https://trac.sagemath.org/ticket/1440#comment:9
<p>
I second that, please apply. I'll open a new ticket for:
</p>
<pre class="wiki">This is somewhat tangential to this issue, but subs has been overridden in a few different classes and the overridden implementation is not quite as robust as (or duplicates code from) the implementation in the element base class. I think that the base implementation/architecture should be strengthened in ways that make these overrides unneeded. Of course, then the subs/substitute synonym should entirely be handled by the base class making it impossible for the inconsistency of the noted bug.
</pre>
TicketmabshoffWed, 04 Jun 2008 20:55:50 GMTstatus changed; resolution set
https://trac.sagemath.org/ticket/1440#comment:10
https://trac.sagemath.org/ticket/1440#comment:10
<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>
</ul>
<p>
Merged redef_substi2.patch in Sage 3.0.3.alpha1
</p>
Ticket