Opened 4 years ago

Closed 4 years ago

#17759 closed enhancement (fixed)

convenience class symbolic ExpressionTreeWalker(Converter)

Reported by: rws Owned by:
Priority: major Milestone: sage-6.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 rws)

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

Used in #9424, #14801, #17849

Change History (20)

comment:1 Changed 4 years ago by rws

  • Branch set to public/17759

comment:2 Changed 4 years ago by rws

  • Commit set to 3781eec1432c7efe6531be4338fef51cc04eda6d
  • Status changed from new to needs_review

New commits:

3781eec17759: convenience class symbolic ExpressionTreeWalker(Converter)

comment:3 Changed 4 years ago by kcrisman

  • Cc kcrisman added

comment:4 Changed 4 years ago by kcrisman

  • Cc niles added

comment:5 Changed 4 years ago by rws

  • Description modified (diff)

comment:6 Changed 4 years ago by rws

  • Description modified (diff)

comment:7 Changed 4 years ago by rws

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:

Last edited 4 years ago by rws (previous) (diff)

comment:8 Changed 4 years ago by rws

  • Status changed from needs_review to needs_work
  • Work issues set to handle hold=True,hypergeometric

comment:9 Changed 4 years ago by git

  • Commit changed from 3781eec1432c7efe6531be4338fef51cc04eda6d to 16aa81d5f5fe7b38db0ce7372e198b6f036ee724

Branch pushed to git repo; I updated commit sha1. New commits:

9375510Merge branch 'develop' into t/17759/public/17759
16aa81d17759: handle hold=True and hypergeometric

comment:10 Changed 4 years ago by rws

  • Milestone changed from sage-6.5 to sage-6.6
  • Status changed from needs_work to needs_review
  • Work issues handle hold=True,hypergeometric deleted

comment:11 Changed 4 years ago by rws

Passes all tests in a 6.6rc1 patchbot run (ignore long startup, docbuild plugins).

comment:12 Changed 4 years ago by rws

  • Dependencies set to 18255

Now some patchbot fails because of #18255.

comment:13 Changed 4 years ago by rws

  • Dependencies changed from 18255 to #18255
  • Milestone changed from sage-6.6 to sage-6.7

comment:14 Changed 4 years ago by rws

  • Branch changed from public/17759 to u/rws/17759

comment:15 Changed 4 years ago by rws

  • Commit changed from 16aa81d5f5fe7b38db0ce7372e198b6f036ee724 to fa73285534e56d5ccd19ffc943237757ea06bcf1
  • Milestone changed from sage-6.7 to sage-6.8

New commits:

fa7328517759: convenience class symbolic ExpressionTreeWalker(Converter)

comment:16 Changed 4 years ago by rws

  • Description modified (diff)

comment:17 follow-up: Changed 4 years ago by mmezzarobba

  • 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 4 years ago by rws

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 4 years ago by rws

Ah no, my mistake, I didn't squash it all, sorry.

comment:20 Changed 4 years ago by vbraun

  • Branch changed from u/rws/17759 to fa73285534e56d5ccd19ffc943237757ea06bcf1
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.