Opened 8 years ago

Closed 8 years ago

#17394 closed defect (fixed)

TypeError in Expression.simplify_hypergeometric()

Reported by: Michael Orlitzky Owned by:
Priority: major Milestone: sage-6.7
Component: symbolics Keywords:
Cc: Ralf Stephan Merged in:
Authors: Nils Bruin Reviewers: Ralf Stephan
Report Upstream: N/A Work issues:
Branch: c16d36f (Commits, GitHub, GitLab) Commit: c16d36f8c9b09bd467d209345df0c965dec5cf0a
Dependencies: Stopgaps:

Status badges

Description

When calling simplify_hypergeometric() on an expression with two variables, sometimes a TypeError is thrown:

sage: y = SR.symbol('y')
sage: f = x*y + x + y
sage: f.simplify_hypergeometric()

produces,

TypeError                                 Traceback (most recent call last)
<ipython-input-4-9e70db185ac4> in <module>()
----> 1 f.simplify_hypergeometric()

/home/mjo/src/sage.git/local/lib/python2.7/site-packages/sage/symbolic/expression.so in sage.symbolic.expression.Expression.simplify_hypergeometric (build/cythonized/sage/symbolic/expression.cpp:40840)()

TypeError: op_add expected 2 arguments, got 3

Change History (15)

comment:1 Changed 8 years ago by Nils Bruin

It's simply this:

sage: A=SR(x+y+x*y)
sage: A.operator()(*A.operands())
TypeError: op_add expected 2 arguments, got 3
sage: A.operator()
sage: A.operator()
<built-in function add>

We should probably not use the built-in function add to mark addition etc., but instead use a variadic version. Same would apply to mul. This issue arises in a lot of places. See for instance maxima_lib.add_vararg. It's probably a change that would need to be made in pynac somewhere.

Other point:

op(*map(lambda o: o.simplify_hypergeometric(algorithm), ops))

is both more compactly and more efficiently expressed in python by

op(*(o.simplify_hypergeometric(algorithm) for o in ops))

Compare:

sage: def l(*args):
....:         return list(args)
....: 
sage: %timeit l( *[a.bit_length() for a in xrange(100)] )
100000 loops, best of 3: 10.2 µs per loop
sage: %timeit l( *(a.bit_length() for a in xrange(100)) )
100000 loops, best of 3: 12.9 µs per loop
sage: %timeit l( *map(lambda a: a.bit_length(), xrange(100)) )
10000 loops, best of 3: 17.9 µs per loop

Basically: don't use "map" in python unless it's considerably more convenient than the corresponding list comprehension.

comment:2 Changed 8 years ago by Nils Bruin

Cc: Ralf Stephan added

comment:3 in reply to:  1 ; Changed 8 years ago by Ralf Stephan

Replying to nbruin:

We should probably not use the built-in function add to mark addition etc., but instead use a variadic version. Same would apply to mul. This issue arises in a lot of places. See for instance maxima_lib.add_vararg. It's probably a change that would need to be made in pynac somewhere.

No, it's all in Expression::operator():

        import operator
        if is_a_add(self._gobj):
            return operator.add

comment:4 in reply to:  3 ; Changed 8 years ago by Nils Bruin

Replying to rws:

Replying to nbruin:

It's probably a change that would need to be made in pynac somewhere.

No, it's all in Expression::operator():

I don't understand the comment. Do you mean "no, that line of code is not in pynac" or do you mean "no we cannot/won't make that change" or something else?

It seems to me you have just located the exact place where the change would need to be made and hence brought this ticket closer to resolution. We would then need to accommodate that change in a lot of other places too, but it probably means code can become simpler in a lot of spots.

comment:5 in reply to:  4 Changed 8 years ago by Nils Bruin

Replying to nbruin:

Replying to rws:

Replying to nbruin:

It's probably a change that would need to be made in pynac somewhere.

No, it's all in Expression::operator():

I don't understand the comment. Do you mean "no, that line of code is not in pynac" or do you mean "no we cannot/won't make that change" or something else?

It seems to me you have just located the exact place where the change would need to be made and hence brought this ticket closer to resolution. We would then need to accommodate that change in a lot of other places too, but it probably means code can become simpler in a lot of spots.

edit: ah, I see. You mean sage.symbolic.expression.Expression.operator. The double colon made me think it was C++ (and search_src doesn't give a hit either, so I was assuming the line appeared in external code). So we can just fix this!

comment:6 Changed 8 years ago by Nils Bruin

Branch: u/nbruin/typeerror_in_expression_simplify_hypergeometric__

comment:7 Changed 8 years ago by Nils Bruin

Commit: 6a3cb27234c22f76062ebc9c7d4ecff6fbb1578a
Status: newneeds_review

This should do the trick

comment:8 Changed 8 years ago by git

Commit: 6a3cb27234c22f76062ebc9c7d4ecff6fbb1578ae7881fec5314b95497a27587e865fa93b939a12e

Branch pushed to git repo; I updated commit sha1. New commits:

e7881fe17394: make sure that `(x+y+z).operator()` returns a function that accepts multiple arguments (and same for multiplication)

comment:9 Changed 8 years ago by Nils Bruin

Authors: Nils Bruin

comment:10 Changed 8 years ago by git

Commit: e7881fec5314b95497a27587e865fa93b939a12e08349c6520dae6fa631728fdadecec81e45a62a5

Branch pushed to git repo; I updated commit sha1. New commits:

08349c617394: doctest that add_vararg and mul_vararg get used by <symbolic-expression>.operator

comment:11 Changed 8 years ago by Ralf Stephan

Milestone: sage-6.5sage-6.7
Reviewers: Ralf Stephan
Status: needs_reviewneeds_work

Looks okay. Patchbot has doctest failures in combinat/finite_state_machine_generators.py however.

comment:12 Changed 8 years ago by git

Commit: 08349c6520dae6fa631728fdadecec81e45a62a5c16d36f8c9b09bd467d209345df0c965dec5cf0a

Branch pushed to git repo; I updated commit sha1. New commits:

c16d36f17394: fixes to finite_state_machine_generators

comment:13 Changed 8 years ago by Nils Bruin

Status: needs_workneeds_review

comment:14 Changed 8 years ago by Ralf Stephan

Status: needs_reviewpositive_review

Is fine now, thanks.

comment:15 Changed 8 years ago by Volker Braun

Branch: u/nbruin/typeerror_in_expression_simplify_hypergeometric__c16d36f8c9b09bd467d209345df0c965dec5cf0a
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.