Opened 6 years ago
Closed 6 years ago
#17394 closed defect (fixed)
TypeError in Expression.simplify_hypergeometric()
Reported by:  mjo  Owned by:  

Priority:  major  Milestone:  sage6.7 
Component:  symbolics  Keywords:  
Cc:  rws  Merged in:  
Authors:  Nils Bruin  Reviewers:  Ralf Stephan 
Report Upstream:  N/A  Work issues:  
Branch:  c16d36f (Commits, GitHub, GitLab)  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) <ipythoninput49e70db185ac4> in <module>() > 1 f.simplify_hypergeometric() /home/mjo/src/sage.git/local/lib/python2.7/sitepackages/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 followup: ↓ 3 Changed 6 years ago by
comment:2 Changed 6 years ago by
 Cc rws added
comment:3 in reply to: ↑ 1 ; followup: ↓ 4 Changed 6 years ago by
Replying to nbruin:
We should probably not use the builtin 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 ; followup: ↓ 5 Changed 6 years ago by
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 6 years ago by
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 6 years ago by
 Branch set to u/nbruin/typeerror_in_expression_simplify_hypergeometric__
comment:7 Changed 6 years ago by
 Commit set to 6a3cb27234c22f76062ebc9c7d4ecff6fbb1578a
 Status changed from new to needs_review
This should do the trick
comment:8 Changed 6 years ago by
 Commit changed from 6a3cb27234c22f76062ebc9c7d4ecff6fbb1578a to e7881fec5314b95497a27587e865fa93b939a12e
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)

comment:9 Changed 6 years ago by
comment:10 Changed 6 years ago by
 Commit changed from e7881fec5314b95497a27587e865fa93b939a12e to 08349c6520dae6fa631728fdadecec81e45a62a5
Branch pushed to git repo; I updated commit sha1. New commits:
08349c6  17394: doctest that add_vararg and mul_vararg get used by <symbolicexpression>.operator

comment:11 Changed 6 years ago by
 Milestone changed from sage6.5 to sage6.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 6 years ago by
 Commit changed from 08349c6520dae6fa631728fdadecec81e45a62a5 to c16d36f8c9b09bd467d209345df0c965dec5cf0a
Branch pushed to git repo; I updated commit sha1. New commits:
c16d36f  17394: fixes to finite_state_machine_generators

comment:13 Changed 6 years ago by
 Status changed from needs_work to needs_review
comment:14 Changed 6 years ago by
 Status changed from needs_review to positive_review
Is fine now, thanks.
comment:15 Changed 6 years ago by
 Branch changed from u/nbruin/typeerror_in_expression_simplify_hypergeometric__ to c16d36f8c9b09bd467d209345df0c965dec5cf0a
 Resolution set to fixed
 Status changed from positive_review to closed
It's simply this:
We should probably not use the builtin 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:
is both more compactly and more efficiently expressed in python by
Compare:
Basically: don't use "map" in python unless it's considerably more convenient than the corresponding list comprehension.