JuliaSymbolics / SymbolicUtils.jl

Symbolic expressions, rewriting and simplification
https://docs.sciml.ai/SymbolicUtils/stable/
Other
545 stars 114 forks source link

Add some PassThroughs to Walk rewriters #272

Closed adamslc closed 3 years ago

adamslc commented 3 years ago

Following a discussion on Slack, this makes Prewalk/Postwalk behave more intuitively by adding some PassThroughs. I'm submitting this as a PR and not an issue because I have some code, but I'm not sure that this is the right approach. Consider:

julia> r1 = @rule sqrt(~x) => (~x)^(1/2)
sqrt(~x) => (~x) ^ (1 / 2)

julia> Prewalk(r1)(sqrt(n)*n)
n^1.5

julia> Prewalk(r1)(exp(n)*n)
n*exp(n)

julia> Prewalk(r1)(n)

So, as you can see, it handles expressions differently based on istree. I'm not sure if this is an issue. It would be easy to make the last case consistent with the first two (i.e. return n). It would be somewhat more computationally expensive to make it always return nothing if the expression has not changed.

shashi commented 3 years ago

Note that adding PassThrough will could result in one more isequal in many cases. For example while running Fixpoint, returning nothing is better. So I think we should have a switch to turn it off internally at least. Or document it properly.

adamslc commented 3 years ago

Good point. It looks like Chain already has an implicit PassThrough behavior, so this won't be an issue in the most common case (applying a set of rules at every level in an expression). Maybe this should be a doc fix instead.

adamslc commented 3 years ago

Closing in favor of #279.