Opened 5 years ago

Closed 4 years ago

#17394 closed defect (fixed)

TypeError in Expression.simplify_hypergeometric()

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

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 follow-up: Changed 5 years ago by nbruin

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 4 years ago by nbruin

  • Cc rws added

comment:3 in reply to: ↑ 1 ; follow-up: Changed 4 years ago by rws

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 ; follow-up: Changed 4 years ago by 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.

comment:5 in reply to: ↑ 4 Changed 4 years ago by nbruin

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 4 years ago by nbruin

  • Branch set to u/nbruin/typeerror_in_expression_simplify_hypergeometric__

comment:7 Changed 4 years ago by nbruin

  • Commit set to 6a3cb27234c22f76062ebc9c7d4ecff6fbb1578a
  • Status changed from new to needs_review

This should do the trick

comment:8 Changed 4 years ago by git

  • Commit changed from 6a3cb27234c22f76062ebc9c7d4ecff6fbb1578a to e7881fec5314b95497a27587e865fa93b939a12e

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 4 years ago by nbruin

  • Authors set to Nils Bruin

comment:10 Changed 4 years ago by git

  • Commit changed from e7881fec5314b95497a27587e865fa93b939a12e to 08349c6520dae6fa631728fdadecec81e45a62a5

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 4 years ago by rws

  • Milestone changed from sage-6.5 to sage-6.7
  • Reviewers set to Ralf Stephan
  • Status changed from needs_review to needs_work

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

comment:12 Changed 4 years ago by git

  • Commit changed from 08349c6520dae6fa631728fdadecec81e45a62a5 to c16d36f8c9b09bd467d209345df0c965dec5cf0a

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

c16d36f17394: fixes to finite_state_machine_generators

comment:13 Changed 4 years ago by nbruin

  • Status changed from needs_work to needs_review

comment:14 Changed 4 years ago by rws

  • Status changed from needs_review to positive_review

Is fine now, thanks.

comment:15 Changed 4 years ago by vbraun

  • Branch changed from u/nbruin/typeerror_in_expression_simplify_hypergeometric__ to c16d36f8c9b09bd467d209345df0c965dec5cf0a
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.