Sage: Ticket #9989: easier access to operands of a symbolic expression
https://trac.sagemath.org/ticket/9989
<p>
Attached patch adds an <code>op</code> attribute to symbolic expressions which gives easy access to its operands. We now have:
</p>
<pre class="wiki">sage: x,y,z = var('x,y,z')
sage: e = x + x*y + z^y + 3*y*z; e
x*y + 3*y*z + z^y + x
sage: e.op[1]
3*y*z
sage: e.op[1,1]
z
sage: e.op[-1]
x
sage: e.op[1:]
[3*y*z, z^y, x]
</pre><p>
Using <code>__getitem__()</code> directly was not an option since it breaks conversion to numpy.
</p>
<p>
Apply <a class="attachment" href="https://trac.sagemath.org/attachment/ticket/9989/trac_9989-operands.take4.patch" title="Attachment 'trac_9989-operands.take4.patch' in Ticket #9989">trac_9989-operands.take4.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/9989/trac_9989-operands.take4.patch" title="Download"></a>
</p>
en-usSagehttps://trac.sagemath.org/chrome/site/logo_sagemath_trac.png
https://trac.sagemath.org/ticket/9989
Trac 1.1.6burcinThu, 23 Sep 2010 22:09:18 GMTattachment set
https://trac.sagemath.org/ticket/9989
https://trac.sagemath.org/ticket/9989
<ul>
<li><strong>attachment</strong>
set to <em>trac_9989-operands.patch</em>
</li>
</ul>
TicketburcinThu, 23 Sep 2010 22:09:59 GMTstatus changed
https://trac.sagemath.org/ticket/9989#comment:1
https://trac.sagemath.org/ticket/9989#comment:1
<ul>
<li><strong>status</strong>
changed from <em>new</em> to <em>needs_review</em>
</li>
</ul>
TicketkcrismanFri, 24 Sep 2010 14:44:47 GMT
https://trac.sagemath.org/ticket/9989#comment:2
https://trac.sagemath.org/ticket/9989#comment:2
<p>
This looks like a good addition, after many sage-support queries - great! I hope to be able to review this eventually, though it will take some time because I am not a Cython expert and would want to be very careful.
</p>
<p>
What is the <code>property</code> thing in <a class="missing wiki">Python/Cython?</a>? I haven't heard of that before (as opposed to <code>def</code> or <code>cdef</code> or <code>class</code>).
</p>
<p>
Does this depend at all on any of the Pynac 0.2.1 tickets' patches?
</p>
TicketburcinFri, 24 Sep 2010 15:19:33 GMT
https://trac.sagemath.org/ticket/9989#comment:3
https://trac.sagemath.org/ticket/9989#comment:3
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/9989#comment:2" title="Comment 2">kcrisman</a>:
</p>
<blockquote class="citation">
<p>
What is the <code>property</code> thing in <a class="missing wiki">Python/Cython?</a>? I haven't heard of that before (as opposed to <code>def</code> or <code>cdef</code> or <code>class</code>).
</p>
</blockquote>
<p>
I also found out about it while looking through the Sage library code for a way to make <code>numpy</code> work when I define <code>__getitem__()</code>:
</p>
<p>
<a class="ext-link" href="http://docs.cython.org/src/userguide/extension_types.html#properties"><span class="icon"></span>http://docs.cython.org/src/userguide/extension_types.html#properties</a>
</p>
<p>
The function defined by <code>__get__()</code> is run when you access that property. This makes the syntax look much cleaner. You don't need to put <code>()</code> at the end any more, compared to the syntax needed for <code>.operands()</code>.
</p>
<blockquote class="citation">
<p>
Does this depend at all on any of the Pynac 0.2.1 tickets' patches?
</p>
</blockquote>
<p>
No, it should be independent. Though I admit that there are a bunch of symbolics patches before this on my queue.
</p>
TicketjpfloriSat, 09 Oct 2010 17:44:43 GMTcc set
https://trac.sagemath.org/ticket/9989#comment:4
https://trac.sagemath.org/ticket/9989#comment:4
<ul>
<li><strong>cc</strong>
<em>jpflori</em> added
</li>
</ul>
TicketrobertwbThu, 16 Dec 2010 16:23:13 GMTstatus changed
https://trac.sagemath.org/ticket/9989#comment:5
https://trac.sagemath.org/ticket/9989#comment:5
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
The error "<a class="missing wiki">TypeError?</a>: cannot index numeric, constant or symbol" is rather obscure, better to make it something like "... has no operands."
</p>
<p>
Any reason why expr.op isn't just a plain list?
</p>
TicketburcinWed, 23 Mar 2011 11:18:44 GMTstatus, description changed; reviewer set
https://trac.sagemath.org/ticket/9989#comment:6
https://trac.sagemath.org/ticket/9989#comment:6
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
<li><strong>reviewer</strong>
set to <em>Robert Bradshaw</em>
</li>
<li><strong>description</strong>
modified (<a href="/ticket/9989?action=diff&version=6">diff</a>)
</li>
</ul>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/9989#comment:5" title="Comment 5">robertwb</a>:
</p>
<blockquote class="citation">
<p>
The error "<a class="missing wiki">TypeError?</a>: cannot index numeric, constant or symbol" is rather obscure, better to make it something like "... has no operands."
</p>
</blockquote>
<p>
Done. The new message is: "expressions containing only a numeric coefficient, constant or symbol have no operands"
</p>
<blockquote class="citation">
<p>
Any reason why expr.op isn't just a plain list?
</p>
</blockquote>
<p>
I wanted to avoid traversing the vector storing the operands and creating a python object for each. A list wouldn't allow nested indexing either:
</p>
<pre class="wiki">sage: x,y,z = var('x,y,z')
sage: e = x + x*y + z^y + 3*y*z; e
x*y + 3*y*z + z^y + x
sage: e.op[1]
3*y*z
sage: e.op[1,1]
z
</pre><p>
This syntax was proposed in a discussion at Sage days 24 last summer.
</p>
<p>
Apply trac_9989-operands.take2.patch
</p>
TicketkcrismanWed, 23 Mar 2011 11:51:39 GMTstatus changed
https://trac.sagemath.org/ticket/9989#comment:7
https://trac.sagemath.org/ticket/9989#comment:7
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_info</em>
</li>
</ul>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/9989#comment:6" title="Comment 6">burcin</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/9989#comment:5" title="Comment 5">robertwb</a>:
</p>
<blockquote class="citation">
<p>
The error "<a class="missing wiki">TypeError?</a>: cannot index numeric, constant or symbol" is rather obscure, better to make it something like "... has no operands."
</p>
</blockquote>
<p>
Done. The new message is: "expressions containing only a numeric coefficient, constant or symbol have no operands"
</p>
</blockquote>
<p>
I think that the part in expression.pyx still has this syntax, and I agree that it is confusing. Putting 'needs info' just in case that is intentional.
</p>
TicketburcinWed, 23 Mar 2011 12:05:07 GMTattachment set
https://trac.sagemath.org/ticket/9989
https://trac.sagemath.org/ticket/9989
<ul>
<li><strong>attachment</strong>
set to <em>trac_9989-operands.take2.patch</em>
</li>
</ul>
TicketburcinWed, 23 Mar 2011 12:06:39 GMTstatus changed
https://trac.sagemath.org/ticket/9989#comment:8
https://trac.sagemath.org/ticket/9989#comment:8
<ul>
<li><strong>status</strong>
changed from <em>needs_info</em> to <em>needs_review</em>
</li>
</ul>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/9989#comment:7" title="Comment 7">kcrisman</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/9989#comment:6" title="Comment 6">burcin</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/9989#comment:5" title="Comment 5">robertwb</a>:
</p>
<blockquote class="citation">
<p>
The error "<a class="missing wiki">TypeError?</a>: cannot index numeric, constant or symbol" is rather obscure, better to make it something like "... has no operands."
</p>
</blockquote>
<p>
Done. The new message is: "expressions containing only a numeric coefficient, constant or symbol have no operands"
</p>
</blockquote>
<p>
I think that the part in expression.pyx still has this syntax, and I agree that it is confusing. Putting 'needs info' just in case that is intentional.
</p>
</blockquote>
<p>
Good catch! I forgot to change that message. Updated patch attached, with same name.
</p>
TicketkcrismanWed, 23 Mar 2011 12:47:52 GMTreviewer changed
https://trac.sagemath.org/ticket/9989#comment:9
https://trac.sagemath.org/ticket/9989#comment:9
<ul>
<li><strong>reviewer</strong>
changed from <em>Robert Bradshaw</em> to <em>Robert Bradshaw, Karl-DIeter Crisman</em>
</li>
</ul>
<p>
Is the <code>normalize_index()</code> business needed because cdef'd things don't accept appropriate negative indices or something? I get what it does, I don't get why it's necessary. Maybe because of the potential for multiple indices to get suboperands?
</p>
<p>
Slowly working my way through it... Cython not being my forte... but looks good so far.
</p>
TicketkcrismanWed, 23 Mar 2011 12:52:02 GMT
https://trac.sagemath.org/ticket/9989#comment:10
https://trac.sagemath.org/ticket/9989#comment:10
<p>
There is no doctest for
</p>
<pre class="wiki">ind_err_msg = "index should either be a slice object, an integer or a list of integers"
</pre><p>
And you might as well just use <code>ind_err_msg</code> in line 139 instead of typing it again.
</p>
TicketkcrismanWed, 23 Mar 2011 15:14:24 GMTstatus changed
https://trac.sagemath.org/ticket/9989#comment:11
https://trac.sagemath.org/ticket/9989#comment:11
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
So far looks good - thanks to <a class="ext-link" href="http://stackoverflow.com/questions/2936863/python-implementing-slicing-in-getitem"><span class="icon"></span>a little help</a> and <a class="ext-link" href="http://www.ginac.de/tutorial/Information-about-expressions.html"><span class="icon"></span>the Ginac refs</a>.
</p>
<p>
Questions, though.
</p>
<ul><li>Regarding the error message:
<pre class="wiki">sage: f = 3*x^2+2*sin(x)-32*sin(ln(x))
sage: f.op[3]
---------------------------------------------------------------------------
IndexError Traceback (most recent call last)
IndexError: operand index out of range, got 3, expect between -3 and 3
</pre>Is this really the error message we want? Here it's the 'exclusive' between, but that could be confusing. Maybe it should say between -3 and 2 (length ops -1)?
</li></ul><ul><li>Next, I wonder whether the following can be supported:
<pre class="wiki">sage: f.op[2:3,3]
TypeError: an integer is required
</pre>since matrices can do this
<pre class="wiki">sage: M = matrix(4,range(16))
sage: M[2:3]
[ 8 9 10 11]
sage: M[2:3,3]
[11]
</pre>The current code ends everything if it's a slice; you just get the operands at that level. But it would be interesting to get the 2nd element of each operand, though perhaps not very useful since you might not know what it is ahead of time. But perhaps for very regular expressions (Christoffel symbol-type surfeit of indices?) it could be useful. We might also want something like <code>sage: M[2:3,3:4]</code> to be supported, such as <code>sage: f.op[2,0:1]</code> instead of having to do
<pre class="wiki">sage: f.op[2].op[0:1]
[sin(log(x))]
</pre>But maybe going back and forth between Ginac and Sage in the way you'd have to for that is tricky.
</li><li>Here is something needed for sure:
<pre class="wiki">sage: f = 3*x^2+2*sin(x)-32*sin(ln(x))
sage: f[1]
---------------------------------------------------------------------------
TypeError Traceback (most recent call last)
TypeError: 'sage.symbolic.expression.Expression' object does not support indexing
sage: search_src('does not support indexing')
<no response>
</pre>We should have at least one doctest <em>somewhere</em> that tests this.
</li></ul><p>
But overall the code is correct for the promised functionality and passes its tests, documented pretty well <em>if</em> you know enough about !GEx etc. So I would say good work, needs work for the last item above and probably the first, and needs info for the second item (matrix-style slices).
</p>
TicketburcinTue, 31 May 2011 13:28:14 GMTstatus, reviewer changed
https://trac.sagemath.org/ticket/9989#comment:12
https://trac.sagemath.org/ticket/9989#comment:12
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
<li><strong>reviewer</strong>
changed from <em>Robert Bradshaw, Karl-DIeter Crisman</em> to <em>Robert Bradshaw, Karl-Dieter Crisman</em>
</li>
</ul>
<p>
Thank you for the thorough reviews. I appreciate the feedback and it's certainly good for someone to look over my changes, since there are often rough edges I fail to see after staring at the code for a while.
</p>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/9989#comment:11" title="Comment 11">kcrisman</a>:
</p>
<blockquote class="citation">
<p>
Questions, though.
</p>
<ul><li>Regarding the error message:
<pre class="wiki">sage: f = 3*x^2+2*sin(x)-32*sin(ln(x))
sage: f.op[3]
---------------------------------------------------------------------------
IndexError Traceback (most recent call last)
IndexError: operand index out of range, got 3, expect between -3 and 3
</pre>Is this really the error message we want? Here it's the 'exclusive' between, but that could be confusing. Maybe it should say between -3 and 2 (length ops -1)?
</li></ul></blockquote>
<p>
Fixed.
</p>
<blockquote class="citation">
<ul><li>Next, I wonder whether the following can be supported:
<pre class="wiki">sage: f.op[2:3,3]
TypeError: an integer is required
</pre>since matrices can do this
<pre class="wiki">sage: M = matrix(4,range(16))
sage: M[2:3]
[ 8 9 10 11]
sage: M[2:3,3]
[11]
</pre>The current code ends everything if it's a slice; you just get the operands at that level. But it would be interesting to get the 2nd element of each operand, though perhaps not very useful since you might not know what it is ahead of time. But perhaps for very regular expressions (Christoffel symbol-type surfeit of indices?) it could be useful. We might also want something like <code>sage: M[2:3,3:4]</code> to be supported, such as <code>sage: f.op[2,0:1]</code> instead of having to do
<pre class="wiki">sage: f.op[2].op[0:1]
[sin(log(x))]
</pre>But maybe going back and forth between Ginac and Sage in the way you'd have to for that is tricky.
</li></ul></blockquote>
<p>
The output is a list if the index is a slice. We could pass further indices to the list's <code>__getitem__()</code> of course, though I'm not convinced this convenience is really a good feature.
</p>
<blockquote class="citation">
<ul><li>Here is something needed for sure:
<pre class="wiki">sage: f = 3*x^2+2*sin(x)-32*sin(ln(x))
sage: f[1]
---------------------------------------------------------------------------
TypeError Traceback (most recent call last)
TypeError: 'sage.symbolic.expression.Expression' object does not support indexing
sage: search_src('does not support indexing')
<no response>
</pre>We should have at least one doctest <em>somewhere</em> that tests this.
</li></ul></blockquote>
<p>
I added a test in the docstring for <code>__get__</code> in <code>expression.pyx</code>.
</p>
<p>
For patchbot:
</p>
<p>
Apply trac_9989-operands.take3.patch
</p>
TicketburcinTue, 31 May 2011 15:10:14 GMTattachment set
https://trac.sagemath.org/ticket/9989
https://trac.sagemath.org/ticket/9989
<ul>
<li><strong>attachment</strong>
set to <em>trac_9989-operands.take3.patch</em>
</li>
</ul>
TicketkcrismanWed, 08 Jun 2011 20:28:19 GMTstatus changed
https://trac.sagemath.org/ticket/9989#comment:13
https://trac.sagemath.org/ticket/9989#comment:13
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
</ul>
<pre class="wiki">Hunk #2 succeeded at 2476 with fuzz 1 (offset -32 lines).
</pre><p>
I certainly don't have time to reread this whole patch again, so <em>assuming</em> that the only changes are the ones mentioned, then tests pass, changes are good, explanation of second point is sufficient, good to go.
</p>
TicketjdemeyerFri, 10 Jun 2011 08:48:48 GMTdescription changed
https://trac.sagemath.org/ticket/9989#comment:14
https://trac.sagemath.org/ticket/9989#comment:14
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/9989?action=diff&version=14">diff</a>)
</li>
</ul>
TicketfbisseySun, 12 Jun 2011 23:17:26 GMT
https://trac.sagemath.org/ticket/9989#comment:15
https://trac.sagemath.org/ticket/9989#comment:15
<p>
*cringe* I am ready to write some documentation if that gets people to use SAGE_LOCAL and SAGE_INC and all the other variables defined in module_list.py
</p>
<pre class="wiki">numpy_depends = [SAGE_LOCAL + '/lib/python/site-packages/numpy/core/include/numpy/_numpyconfig.h']
flint_depends = [SAGE_LOCAL + '/include/FLINT/flint.h']
singular_depends = [SAGE_LOCAL + '/include/libsingular.h']
ginac_depends = [SAGE_LOCAL + '/include/pynac/ginac.h']
</pre><blockquote>
<p>
rather than
</p>
<pre class="wiki">SAGE_ROOT + "/local/include/pynac/ginac.h"
</pre></blockquote>
<p>
and other combinations of SAGE_ROOT + "/local..."
</p>
TicketburcinMon, 13 Jun 2011 17:15:45 GMTdescription changed
https://trac.sagemath.org/ticket/9989#comment:16
https://trac.sagemath.org/ticket/9989#comment:16
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/9989?action=diff&version=16">diff</a>)
</li>
</ul>
<p>
Good catch. Thanks, Francois.
</p>
<p>
I uploaded a new patch that just changes <code>module_list.py</code> to use <code>SAGE_LOCAL</code> instead of <code>SAGE_ROOT</code>.
</p>
<p>
Francois, why don't you open a ticket to rename <code>SAGE_ROOT</code> in <code>module_list.py</code> to something like <code>SAGE_ROOT_DO_NOT_USE</code> with a comment above it to point out why <code>SAGE_LOCAL</code> is better.
</p>
TicketfbisseyMon, 13 Jun 2011 19:04:06 GMT
https://trac.sagemath.org/ticket/9989#comment:17
https://trac.sagemath.org/ticket/9989#comment:17
<p>
Actually you could have used the ginac_depends variable. But your suggestion is good I can make it a part of <a class="closed ticket" href="https://trac.sagemath.org/ticket/11377" title="enhancement: Clean and harmonize module_list.py (closed: fixed)">#11377</a>. I think I may try to write a little bit about these variables in the manual.
</p>
TicketburcinTue, 14 Jun 2011 00:15:07 GMTattachment set
https://trac.sagemath.org/ticket/9989
https://trac.sagemath.org/ticket/9989
<ul>
<li><strong>attachment</strong>
set to <em>trac_9989-operands.take4.patch</em>
</li>
</ul>
TicketburcinTue, 14 Jun 2011 00:15:44 GMT
https://trac.sagemath.org/ticket/9989#comment:18
https://trac.sagemath.org/ticket/9989#comment:18
<p>
Apply trac_9989-operands.take4.patch
</p>
TicketvbraunTue, 14 Jun 2011 00:42:45 GMTreviewer changed; keywords set
https://trac.sagemath.org/ticket/9989#comment:19
https://trac.sagemath.org/ticket/9989#comment:19
<ul>
<li><strong>keywords</strong>
<em>sd31</em> added
</li>
<li><strong>reviewer</strong>
changed from <em>Robert Bradshaw, Karl-Dieter Crisman</em> to <em>Robert Bradshaw, Karl-Dieter Crisman, Volker Braun</em>
</li>
</ul>
<p>
By private communication, I asked Burcin to change the <code>_repr_()</code> of the operand wrapper to just <code>Operands of x^2</code>. I think its looks great now.
</p>
TicketjdemeyerTue, 14 Jun 2011 17:29:33 GMTstatus changed
https://trac.sagemath.org/ticket/9989#comment:20
https://trac.sagemath.org/ticket/9989#comment:20
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>needs_work</em>
</li>
</ul>
TicketjdemeyerTue, 14 Jun 2011 17:30:47 GMTstatus changed
https://trac.sagemath.org/ticket/9989#comment:21
https://trac.sagemath.org/ticket/9989#comment:21
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
<p>
Somebody should review the new patch... (I am not sure whether Volker's comment "I think its looks great now." means a formal positive_review)
</p>
TicketvbraunTue, 14 Jun 2011 17:40:01 GMTstatus changed
https://trac.sagemath.org/ticket/9989#comment:22
https://trac.sagemath.org/ticket/9989#comment:22
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
</ul>
<p>
Yes, positive review!
</p>
TicketjdemeyerWed, 15 Jun 2011 15:23:46 GMTstatus changed; resolution, merged set
https://trac.sagemath.org/ticket/9989#comment:23
https://trac.sagemath.org/ticket/9989#comment:23
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>closed</em>
</li>
<li><strong>resolution</strong>
set to <em>fixed</em>
</li>
<li><strong>merged</strong>
set to <em>sage-4.7.1.alpha4</em>
</li>
</ul>
Ticket