constructor-level simplification

[shashi]

This design is taken from SymEngine. It is similar to what I had going on here: https://github.com/shashi/ModelingToolkit.jl/blob/s/simplification/src/polyform.jl with optimizations for the simplest cases.

fixes #151

todo:

• fix output symtype.

[shashi]

@YingboMa what was your benchmark big expression?

[YingboMa]

To generate a big expression, I recommend to use

julia> using LinearAlgebra, ModelingToolkit

julia> @variables X[1:5, 1:5];

julia> det(lu(X))

[YingboMa]

@shashi there was

+(a::Number, b::Add) = iszero(a) ? b : Add(a, make_add_dict(1, b))

before and I changed it to

+(a::Number, b::Add) = iszero(a) ? b : Add(a + b.coeff, b.dict)

When b is already an Add does it still need make_add_dict(1, b)? I thought we only need make_add_dict(1, b) when b is not an Add to canonicalize it.

[YingboMa]

BTW, we used

macro bigexpr()
    ex = :(x + y + z + z * y + o * x + x)
    bigex = Expr(:call,)
    bigex.args = [:+; mapreduce(_->ex.args[2:end], vcat, 1:200)];
    esc(bigex)
end

to benchmark construction speed before.

SymEngine:

julia> using SymEngine

julia> @vars x y z o
(x, y, z, o)

julia> @time @bigexpr
  0.001075 seconds (2.80 k allocations: 62.609 KiB)
400*x + 200*y + 200*z + 200*o*x + 200*y*z

SymbolicUtils:

julia> using SymbolicUtils

julia> @syms x y z o
(x, y, z, o)

julia> @time @bigexpr
  0.001439 seconds (35.57 k allocations: 2.088 MiB)
200o * x + 200y + 200z + 200y * z + 400x

SymbolicUtils master:

julia> @time simplify(expr, polynorm=true)
  0.162938 seconds (3.59 M allocations: 64.250 MiB, 1.61% gc time)
(400 * x) + (200 * (y + z)) + (200 * o * x) + (200 * y * z)

PS: I did run twice to make sure that there's no compile time.

[codecov-io]

Codecov Report

Merging #154 (e2517f5) into master (e00284a) will increase coverage by 0.37%.
The diff coverage is 96.02%.

@@            Coverage Diff             @@
##           master     #154      +/-   ##
==========================================
+ Coverage   91.55%   91.93%   +0.37%     
==========================================
  Files          11       11              
  Lines         687      818     +131     
==========================================
+ Hits          629      752     +123     
- Misses         58       66       +8     

Impacted Files	Coverage Δ
src/SymbolicUtils.jl	100.00% <ø> (ø)
src/types.jl	91.63% <95.59%> (+5.92%)	⬆️
src/methods.jl	89.55% <100.00%> (+2.05%)	⬆️
src/rewriters.jl	88.00% <100.00%> (+1.23%)	⬆️
src/rule.jl	89.10% <100.00%> (-1.07%)	⬇️
src/ordering.jl	94.87% <0.00%> (-2.66%)	⬇️
src/matchers.jl	96.15% <0.00%> (-2.10%)	⬇️
src/simplify_rules.jl	81.81% <0.00%> (-1.52%)	⬇️
src/utils.jl	90.10% <0.00%> (-0.90%)	⬇️
... and 5 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e00284a...e2517f5. Read the comment docs.

[shashi]

fixes #151

[shashi]

@cscherrer @david-pl pinging you both since you have been using this library!

+, -, *, /, ^ on <:Symbolic now return Add, Mul or Pow which are subtype of Symbolic. All other previously defined functions still return Term.

The interface of operation(x) and arguments(x) is defined on all these types, and istree(x) = true for all of them.

If you never access the fields of a term or never dispatch on Term in your code, nothing should change for you.

Now instead of dispatching on Term, use the if istree(x) to also accept Add Mul and Pow... this should not have performance penalty, and it does not harm flexibility a lot since you can still also use dispatch.

EDIT: + and * on Symbolic{<:Any} are assumed to be commutative and associative by default ON construction. so you may want to define your own + or * methods for Symbolic{<:YourType} objects to return Term -- this would be the default before this change.

@rules should continue to work as they did before.

Hopefully this helps, feel free to ping me for help any time!

[shashi]

• and * on Symbolic{<:Any} are assumed to be commutative and associative by default

This should have been restricted to Symbolic{<:Number}, but see the issue here #155 which makes that not yet feasible...

[cscherrer]

Thanks @shashi! This reminds me of a question. It's a little bit of a tangent, so I'll ask in the Zulip and edit this message with a link.

EDIT: Here it is: https://julialang.zulipchat.com/#narrow/stream/236639-symbolic-programming/topic/Constructor-level.20simplification/near/222106581

[TymonKilich]

• and * on Symbolic{<:Any} are assumed to be commutative and associative by default

This should have been restricted to Symbolic{<:Number}, but see the issue here #155 which makes that not yet feasible...

It certainly surprised me, so maybe I should put a reference to this PR with information about that behaviour in manual/page?

How are the perspectives for solving this issue looking? I'm wondering whether to put a note about that behaviour in manual as for me it's certainly surprising that e.g.

julia> @syms f() g()
(f()::Number, g()::Number)

julia> 2g + g*f + f*g + g
3g()::Number + 2f()::Number*g()::Number

of course I suppose that it's mostly ok for the functions that return numbers (at least can't think of the reason why it wouldn't be ATM), since * ≢ ∘ but is still a bit of surprise 😄

[shashi]

This should have been restricted to Symbolic{<:Number}, but see the issue here #155 which makes that not yet feasible...

How are the perspectives for solving this issue looking? I'm wondering whether to put a note about that behaviour in manual as for me it's certainly surprising that e.g.

For those looking at this now, it was fixed in #259 👍