TypeError in Expression.simplify_hypergeometric()

[orlitzky]

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

CC: @rwst

Component: symbolics

Author: Nils Bruin

Branch/Commit: c16d36f

Reviewer: Ralf Stephan

Issue created by migration from https://trac.sagemath.org/ticket/17394

[nbruin]

comment:1

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.

[rwst]

comment:3

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

[nbruin]

comment:4

Replying to @rwst:

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.

[nbruin]

comment:5

Replying to @nbruin:

Replying to @rwst:

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!

[nbruin]

Branch: u/nbruin/typeerror_in_expression_simplify_hypergeometric__

[nbruin]

Commit: 6a3cb27

[nbruin]

comment:7

This should do the trick

[sagetrac-git]

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

e7881fe

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

[sagetrac-git]

Changed commit from 6a3cb27 to e7881fe

[nbruin]

Author: Nils Bruin