Opened 12 years ago
Closed 11 years ago
#9989 closed enhancement (fixed)
easier access to operands of a symbolic expression
Reported by: | Burcin Erocal | Owned by: | Burcin Erocal |
---|---|---|---|
Priority: | major | Milestone: | sage-4.7.1 |
Component: | symbolics | Keywords: | sd31 |
Cc: | Jean-Pierre Flori | Merged in: | sage-4.7.1.alpha4 |
Authors: | Burcin Erocal | Reviewers: | Robert Bradshaw, Karl-Dieter Crisman, Volker Braun |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
Attached patch adds an op
attribute to symbolic expressions which gives easy access to its operands. We now have:
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]
Using __getitem__()
directly was not an option since it breaks conversion to numpy.
Attachments (4)
Change History (27)
Changed 12 years ago by
Attachment: | trac_9989-operands.patch added |
---|
comment:1 Changed 12 years ago by
Status: | new → needs_review |
---|
comment:2 follow-up: 3 Changed 12 years ago by
comment:3 Changed 12 years ago by
Replying to kcrisman:
What is the
property
thing in Python/Cython?? I haven't heard of that before (as opposed todef
orcdef
orclass
).
I also found out about it while looking through the Sage library code for a way to make numpy
work when I define __getitem__()
:
http://docs.cython.org/src/userguide/extension_types.html#properties
The function defined by __get__()
is run when you access that property. This makes the syntax look much cleaner. You don't need to put ()
at the end any more, compared to the syntax needed for .operands()
.
Does this depend at all on any of the Pynac 0.2.1 tickets' patches?
No, it should be independent. Though I admit that there are a bunch of symbolics patches before this on my queue.
comment:4 Changed 12 years ago by
Cc: | Jean-Pierre Flori added |
---|
comment:5 follow-up: 6 Changed 12 years ago by
Status: | needs_review → needs_work |
---|
The error "TypeError?: cannot index numeric, constant or symbol" is rather obscure, better to make it something like "... has no operands."
Any reason why expr.op isn't just a plain list?
comment:6 follow-up: 7 Changed 12 years ago by
Description: | modified (diff) |
---|---|
Reviewers: | → Robert Bradshaw |
Status: | needs_work → needs_review |
Replying to robertwb:
The error "TypeError?: cannot index numeric, constant or symbol" is rather obscure, better to make it something like "... has no operands."
Done. The new message is: "expressions containing only a numeric coefficient, constant or symbol have no operands"
Any reason why expr.op isn't just a plain list?
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:
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
This syntax was proposed in a discussion at Sage days 24 last summer.
Apply trac_9989-operands.take2.patch
comment:7 follow-up: 8 Changed 12 years ago by
Status: | needs_review → needs_info |
---|
Replying to burcin:
Replying to robertwb:
The error "TypeError?: cannot index numeric, constant or symbol" is rather obscure, better to make it something like "... has no operands."
Done. The new message is: "expressions containing only a numeric coefficient, constant or symbol have no operands"
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.
Changed 12 years ago by
Attachment: | trac_9989-operands.take2.patch added |
---|
comment:8 Changed 12 years ago by
Status: | needs_info → needs_review |
---|
Replying to kcrisman:
Replying to burcin:
Replying to robertwb:
The error "TypeError?: cannot index numeric, constant or symbol" is rather obscure, better to make it something like "... has no operands."
Done. The new message is: "expressions containing only a numeric coefficient, constant or symbol have no operands"
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.
Good catch! I forgot to change that message. Updated patch attached, with same name.
comment:9 Changed 12 years ago by
Reviewers: | Robert Bradshaw → Robert Bradshaw, Karl-DIeter Crisman |
---|
Is the normalize_index()
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?
Slowly working my way through it... Cython not being my forte... but looks good so far.
comment:10 Changed 12 years ago by
There is no doctest for
ind_err_msg = "index should either be a slice object, an integer or a list of integers"
And you might as well just use ind_err_msg
in line 139 instead of typing it again.
comment:11 follow-up: 12 Changed 12 years ago by
Status: | needs_review → needs_work |
---|
So far looks good - thanks to a little help and the Ginac refs.
Questions, though.
- Regarding the error message:
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
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)?
- Next, I wonder whether the following can be supported:
sage: f.op[2:3,3] TypeError: an integer is required
since matrices can do thissage: M = matrix(4,range(16)) sage: M[2:3] [ 8 9 10 11] sage: M[2:3,3] [11]
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 likesage: M[2:3,3:4]
to be supported, such assage: f.op[2,0:1]
instead of having to dosage: f.op[2].op[0:1] [sin(log(x))]
But maybe going back and forth between Ginac and Sage in the way you'd have to for that is tricky. - Here is something needed for sure:
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>
We should have at least one doctest somewhere that tests this.
But overall the code is correct for the promised functionality and passes its tests, documented pretty well if 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).
comment:12 Changed 12 years ago by
Reviewers: | Robert Bradshaw, Karl-DIeter Crisman → Robert Bradshaw, Karl-Dieter Crisman |
---|---|
Status: | needs_work → needs_review |
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.
Replying to kcrisman:
Questions, though.
- Regarding the error message:
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 3Is 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)?
Fixed.
- Next, I wonder whether the following can be supported:
sage: f.op[2:3,3] TypeError: an integer is requiredsince matrices can do thissage: M = matrix(4,range(16)) sage: M[2:3] [ 8 9 10 11] sage: M[2:3,3] [11]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 likesage: M[2:3,3:4]
to be supported, such assage: f.op[2,0:1]
instead of having to dosage: f.op[2].op[0:1] [sin(log(x))]But maybe going back and forth between Ginac and Sage in the way you'd have to for that is tricky.
The output is a list if the index is a slice. We could pass further indices to the list's __getitem__()
of course, though I'm not convinced this convenience is really a good feature.
- Here is something needed for sure:
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>We should have at least one doctest somewhere that tests this.
I added a test in the docstring for __get__
in expression.pyx
.
For patchbot:
Apply trac_9989-operands.take3.patch
Changed 12 years ago by
Attachment: | trac_9989-operands.take3.patch added |
---|
comment:13 Changed 11 years ago by
Status: | needs_review → positive_review |
---|
Hunk #2 succeeded at 2476 with fuzz 1 (offset -32 lines).
I certainly don't have time to reread this whole patch again, so assuming that the only changes are the ones mentioned, then tests pass, changes are good, explanation of second point is sufficient, good to go.
comment:14 Changed 11 years ago by
Description: | modified (diff) |
---|
comment:15 Changed 11 years ago by
*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
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']
rather than
SAGE_ROOT + "/local/include/pynac/ginac.h"
and other combinations of SAGE_ROOT + "/local..."
comment:16 Changed 11 years ago by
Description: | modified (diff) |
---|
Good catch. Thanks, Francois.
I uploaded a new patch that just changes module_list.py
to use SAGE_LOCAL
instead of SAGE_ROOT
.
Francois, why don't you open a ticket to rename SAGE_ROOT
in module_list.py
to something like SAGE_ROOT_DO_NOT_USE
with a comment above it to point out why SAGE_LOCAL
is better.
comment:17 Changed 11 years ago by
Actually you could have used the ginac_depends variable. But your suggestion is good I can make it a part of #11377. I think I may try to write a little bit about these variables in the manual.
Changed 11 years ago by
Attachment: | trac_9989-operands.take4.patch added |
---|
comment:19 Changed 11 years ago by
Keywords: | sd31 added |
---|---|
Reviewers: | Robert Bradshaw, Karl-Dieter Crisman → Robert Bradshaw, Karl-Dieter Crisman, Volker Braun |
By private communication, I asked Burcin to change the _repr_()
of the operand wrapper to just Operands of x^2
. I think its looks great now.
comment:20 Changed 11 years ago by
Status: | positive_review → needs_work |
---|
comment:21 Changed 11 years ago by
Status: | needs_work → needs_review |
---|
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)
comment:23 Changed 11 years ago by
Merged in: | → sage-4.7.1.alpha4 |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
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.
What is the
property
thing in Python/Cython?? I haven't heard of that before (as opposed todef
orcdef
orclass
).Does this depend at all on any of the Pynac 0.2.1 tickets' patches?