Opened 5 years ago
Closed 5 years ago
#17759 closed enhancement (fixed)
convenience class symbolic ExpressionTreeWalker(Converter)
Reported by:  rws  Owned by:  

Priority:  major  Milestone:  sage6.8 
Component:  symbolics  Keywords:  
Cc:  kcrisman, niles  Merged in:  
Authors:  Ralf Stephan  Reviewers:  Marc Mezzarobba 
Report Upstream:  N/A  Work issues:  
Branch:  fa73285 (Commits)  Commit:  fa73285534e56d5ccd19ffc943237757ea06bcf1 
Dependencies:  #18255  Stopgaps: 
Description (last modified by )
This class can already be used as base class for SubstituteFunction
but is intended also for other uses.
sage: from sage.symbolic.expression_conversions import ExpressionTreeWalker sage: from sage.symbolic.random_tests import random_expr sage: foo = random_expr(20, nvars=2) sage: s = ExpressionTreeWalker(foo) sage: bool(s() == foo) True
Change History (20)
comment:1 Changed 5 years ago by
 Branch set to public/17759
comment:2 Changed 5 years ago by
 Commit set to 3781eec1432c7efe6531be4338fef51cc04eda6d
 Status changed from new to needs_review
comment:3 Changed 5 years ago by
 Cc kcrisman added
comment:4 Changed 5 years ago by
 Cc niles added
comment:5 Changed 5 years ago by
 Description modified (diff)
comment:6 Changed 5 years ago by
 Description modified (diff)
comment:7 Changed 5 years ago by
OK, this is not really a tree walker but a deep copier, i.e., function objects are called again. This misses possible hold arguments that were given but are lost, see #17849. So let's implement a real tree walker in this ticket that only replaces specific operators but leaves the rest of the tree as is.
EDIT:
comment:8 Changed 5 years ago by
 Status changed from needs_review to needs_work
 Work issues set to handle hold=True,hypergeometric
comment:9 Changed 5 years ago by
 Commit changed from 3781eec1432c7efe6531be4338fef51cc04eda6d to 16aa81d5f5fe7b38db0ce7372e198b6f036ee724
comment:10 Changed 5 years ago by
 Milestone changed from sage6.5 to sage6.6
 Status changed from needs_work to needs_review
 Work issues handle hold=True,hypergeometric deleted
comment:11 Changed 5 years ago by
Passes all tests in a 6.6rc1 patchbot run (ignore long startup, docbuild plugins).
comment:12 Changed 5 years ago by
 Dependencies set to 18255
Now some patchbot fails because of #18255.
comment:13 Changed 5 years ago by
 Dependencies changed from 18255 to #18255
 Milestone changed from sage6.6 to sage6.7
comment:14 Changed 5 years ago by
 Branch changed from public/17759 to u/rws/17759
comment:15 Changed 5 years ago by
 Commit changed from 16aa81d5f5fe7b38db0ce7372e198b6f036ee724 to fa73285534e56d5ccd19ffc943237757ea06bcf1
 Milestone changed from sage6.7 to sage6.8
New commits:
fa73285  17759: convenience class symbolic ExpressionTreeWalker(Converter)

comment:16 Changed 5 years ago by
 Description modified (diff)
comment:17 followup: ↓ 18 Changed 5 years ago by
 Reviewers set to Marc Mezzarobba
 Status changed from needs_review to positive_review
Lgtm, though I don't know that code too well... (and the commit history is a little bit confusing :)
)
comment:18 in reply to: ↑ 17 Changed 5 years ago by
Replying to mmezzarobba:
Lgtm, though I don't know that code too well... (and the commit history is a little bit confusing
:)
)
Changing the branch should make earlier commits disappear, at least in the commit log. Do we have a trac bug ticket for this?
But thanks for the review!
comment:19 Changed 5 years ago by
Ah no, my mistake, I didn't squash it all, sorry.
comment:20 Changed 5 years ago by
 Branch changed from u/rws/17759 to fa73285534e56d5ccd19ffc943237757ea06bcf1
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
17759: convenience class symbolic ExpressionTreeWalker(Converter)