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:  sage6.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: 
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 8 years ago by
comment:2 Changed 8 years ago by
Cc:  Ralf Stephan added 

comment:3 followup: 4 Changed 8 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 followup: 5 Changed 8 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 Changed 8 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 8 years ago by
Branch:  → u/nbruin/typeerror_in_expression_simplify_hypergeometric__ 

comment:7 Changed 8 years ago by
Commit:  → 6a3cb27234c22f76062ebc9c7d4ecff6fbb1578a 

Status:  new → needs_review 
This should do the trick
comment:8 Changed 8 years ago by
Commit:  6a3cb27234c22f76062ebc9c7d4ecff6fbb1578a → 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 8 years ago by
Authors:  → Nils Bruin 

comment:10 Changed 8 years ago by
Commit:  e7881fec5314b95497a27587e865fa93b939a12e → 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 8 years ago by
Milestone:  sage6.5 → sage6.7 

Reviewers:  → Ralf Stephan 
Status:  needs_review → needs_work 
Looks okay. Patchbot has doctest failures in combinat/finite_state_machine_generators.py
however.
comment:12 Changed 8 years ago by
Commit:  08349c6520dae6fa631728fdadecec81e45a62a5 → c16d36f8c9b09bd467d209345df0c965dec5cf0a 

Branch pushed to git repo; I updated commit sha1. New commits:
c16d36f  17394: fixes to finite_state_machine_generators

comment:13 Changed 8 years ago by
Status:  needs_work → needs_review 

comment:15 Changed 8 years ago by
Branch:  u/nbruin/typeerror_in_expression_simplify_hypergeometric__ → c16d36f8c9b09bd467d209345df0c965dec5cf0a 

Resolution:  → fixed 
Status:  positive_review → 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.