Opened 7 years ago
Closed 6 years ago
#19464 closed defect (fixed)
ExpressionTreeWalker fails on some functions
Reported by:  Ralf Stephan  Owned by:  

Priority:  major  Milestone:  sageduplicate/invalid/wontfix 
Component:  symbolics  Keywords:  
Cc:  Merged in:  
Authors:  Ralf Stephan  Reviewers:  
Report Upstream:  N/A  Work issues:  
Branch:  u/rws/expressiontreewalker_fails_on_some_functions (Commits, GitHub, GitLab)  Commit:  d2afc44b63a8247e70dba2a32d823cdb5c49c7bc 
Dependencies:  Stopgaps: 
Description (last modified by )
In some tickets (eg #15024, #16813) this doctest from symbolic/expression_conversions.py
sage: foo = random_expr(20, nvars=2) sage: foo sage: s = ExpressionTreeWalker(foo) sage: bool(s() == foo)
fails because the set of functions returned by random_expr
contains one of floor
/ceil
which currently don't accept the hold
keyword:
sage: floor(x,hold=True)  TypeError Traceback (most recent call last) <ipythoninput8fc5809e0a430> in <module>() > 1 floor(x,hold=True) TypeError: __call__() got an unexpected keyword argument 'hold'
This would affect any use of the walker or its subclasses on floor
expressions.
The reason is that both functions handle their calls themselves (instead of relying on superclass functionality) because at the time it was deemed necessary to provide a keyword named maximum_bits
.
Change History (9)
comment:1 Changed 7 years ago by
Description:  modified (diff) 

comment:2 Changed 7 years ago by
Description:  modified (diff) 

comment:3 Changed 7 years ago by
Description:  modified (diff) 

comment:4 Changed 7 years ago by
Branch:  → u/rws/expressiontreewalker_fails_on_some_functions 

comment:5 Changed 7 years ago by
Authors:  → Ralf Stephan 

Commit:  → 4198c28765690dca3ee6ae80d3f9a7f021f3d919 
Status:  new → needs_review 
comment:6 Changed 7 years ago by
Commit:  4198c28765690dca3ee6ae80d3f9a7f021f3d919 → d2afc44b63a8247e70dba2a32d823cdb5c49c7bc 

Branch pushed to git repo; I updated commit sha1. New commits:
d2afc44  19464: fix bug

comment:7 Changed 6 years ago by
There is an incompatibility with #12121 which also fixes the issue. The ticket completely removes the handmade __call__
for both floor
and ceil
.
comment:8 Changed 6 years ago by
Milestone:  sage6.10 → sageduplicate/invalid/wontfix 

Status:  needs_review → positive_review 
OK, duplicate.
comment:9 Changed 6 years ago by
Resolution:  → fixed 

Status:  positive_review → closed 
The changes make
floor
/ceil
accept thehold
keyword and hand it over to the superclasses. It has no visible effect except fixing the bug, at the moment.New commits:
fix 19464 by allowing a hold keyword on floor/ceil