Opened 9 years ago
Closed 8 years ago
#9989 closed enhancement (fixed)
easier access to operands of a symbolic expression
Reported by: | burcin | Owned by: | burcin |
---|---|---|---|
Priority: | major | Milestone: | sage-4.7.1 |
Component: | symbolics | Keywords: | sd31 |
Cc: | jpflori | 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 9 years ago by
comment:1 Changed 9 years ago by
- Status changed from new to needs_review
comment:2 follow-up: ↓ 3 Changed 9 years ago by
comment:3 in reply to: ↑ 2 Changed 9 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 9 years ago by
- Cc jpflori added
comment:5 follow-up: ↓ 6 Changed 9 years ago by
- Status changed from needs_review to 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 in reply to: ↑ 5 ; follow-up: ↓ 7 Changed 9 years ago by
- Description modified (diff)
- Reviewers set to Robert Bradshaw
- Status changed from needs_work to 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 in reply to: ↑ 6 ; follow-up: ↓ 8 Changed 9 years ago by
- Status changed from needs_review to 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 9 years ago by
comment:8 in reply to: ↑ 7 Changed 9 years ago by
- Status changed from needs_info to 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 9 years ago by
- Reviewers changed from Robert Bradshaw to 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 9 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 9 years ago by
- Status changed from needs_review to 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 in reply to: ↑ 11 Changed 9 years ago by
- Reviewers changed from Robert Bradshaw, Karl-DIeter Crisman to Robert Bradshaw, Karl-Dieter Crisman
- Status changed from needs_work to 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 9 years ago by
comment:13 Changed 9 years ago by
- Status changed from needs_review to 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 9 years ago by
- Description modified (diff)
comment:15 Changed 9 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 9 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 9 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 9 years ago by
comment:18 Changed 9 years ago by
Apply trac_9989-operands.take4.patch
comment:19 Changed 9 years ago by
- Keywords sd31 added
- Reviewers changed from Robert Bradshaw, Karl-Dieter Crisman to 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 8 years ago by
- Status changed from positive_review to needs_work
comment:21 Changed 8 years ago by
- Status changed from needs_work to 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:22 Changed 8 years ago by
- Status changed from needs_review to positive_review
Yes, positive review!
comment:23 Changed 8 years ago by
- Merged in set to sage-4.7.1.alpha4
- Resolution set to fixed
- Status changed from positive_review to 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?