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 burcin)

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.

Apply trac_9989-operands.take4.patch

Attachments (4)

trac_9989-operands.patch (11.4 KB) - added by burcin 9 years ago.
trac_9989-operands.take2.patch (11.6 KB) - added by burcin 9 years ago.
trac_9989-operands.take3.patch (12.2 KB) - added by burcin 9 years ago.
trac_9989-operands.take4.patch (12.1 KB) - added by burcin 9 years ago.

Download all attachments as: .zip

Change History (27)

Changed 9 years ago by burcin

comment:1 Changed 9 years ago by burcin

  • Status changed from new to needs_review

comment:2 follow-up: Changed 9 years ago by kcrisman

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 to def or cdef or class).

Does this depend at all on any of the Pynac 0.2.1 tickets' patches?

comment:3 in reply to: ↑ 2 Changed 9 years ago by burcin

Replying to kcrisman:

What is the property thing in Python/Cython?? I haven't heard of that before (as opposed to def or cdef or class).

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 jpflori

  • Cc jpflori added

comment:5 follow-up: Changed 9 years ago by robertwb

  • 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: Changed 9 years ago by burcin

  • 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: Changed 9 years ago by kcrisman

  • 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 burcin

comment:8 in reply to: ↑ 7 Changed 9 years ago by burcin

  • 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 kcrisman

  • 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 kcrisman

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: Changed 9 years ago by kcrisman

  • 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 this
    sage: 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 like sage: M[2:3,3:4] to be supported, such as sage: f.op[2,0:1] instead of having to do
    sage: 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 burcin

  • 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 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)?

Fixed.

  • Next, I wonder whether the following can be supported:
    sage: f.op[2:3,3]
    TypeError: an integer is required
    
    since matrices can do this
    sage: 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 like sage: M[2:3,3:4] to be supported, such as sage: f.op[2,0:1] instead of having to do
    sage: 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 burcin

comment:13 Changed 9 years ago by kcrisman

  • 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 jdemeyer

  • Description modified (diff)

comment:15 Changed 9 years ago by fbissey

*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 burcin

  • 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 fbissey

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 burcin

comment:18 Changed 9 years ago by burcin

Apply trac_9989-operands.take4.patch

comment:19 Changed 9 years ago by vbraun

  • 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 jdemeyer

  • Status changed from positive_review to needs_work

comment:21 Changed 8 years ago by jdemeyer

  • 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 vbraun

  • Status changed from needs_review to positive_review

Yes, positive review!

comment:23 Changed 8 years ago by jdemeyer

  • Merged in set to sage-4.7.1.alpha4
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.