Opened 6 years ago
Closed 5 years ago
#19464 closed defect (fixed)
ExpressionTreeWalker fails on some functions
Reported by:  rws  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 5 years ago by
 Description modified (diff)
comment:2 Changed 5 years ago by
 Description modified (diff)
comment:3 Changed 5 years ago by
 Description modified (diff)
comment:4 Changed 5 years ago by
 Branch set to u/rws/expressiontreewalker_fails_on_some_functions
comment:5 Changed 5 years ago by
 Commit set to 4198c28765690dca3ee6ae80d3f9a7f021f3d919
 Status changed from new to needs_review
comment:6 Changed 5 years ago by
 Commit changed from 4198c28765690dca3ee6ae80d3f9a7f021f3d919 to d2afc44b63a8247e70dba2a32d823cdb5c49c7bc
Branch pushed to git repo; I updated commit sha1. New commits:
d2afc44  19464: fix bug

comment:7 Changed 5 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 5 years ago by
 Milestone changed from sage6.10 to sageduplicate/invalid/wontfix
 Status changed from needs_review to positive_review
OK, duplicate.
comment:9 Changed 5 years ago by
 Resolution set to fixed
 Status changed from positive_review to 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