Refactor degree & coeff for Add & Mul

[bowenszhu]

1. avoids creating a vector due to broadcast
2. avoids sorting arguments
3. make use of the data structures of Mul and Add

[codecov-commenter]

Codecov Report

Merging #685 (429377e) into master (c893846) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #685      +/-   ##
==========================================
+ Coverage   76.49%   76.52%   +0.02%     
==========================================
  Files          23       23              
  Lines        2719     2722       +3     
==========================================
+ Hits         2080     2083       +3     
  Misses        639      639              

Impacted Files	Coverage Δ
src/utils.jl	76.33% <100.00%> (+0.55%)	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[t-bltg]

Have you checked that this is effectively more efficient ?
From my measurements, it seems that this PR makes it worse and the rewrite of degree(p::Mul, ...) in #677 is more efficient:

using BenchmarkTools, LinearAlgebra
import SymbolicUtils: Mul
using Symbolics

main() = begin
  @variables x
  e = (2x + 1) * (3x - 1) * (4x^2 - 1) |> expand
  # old implementation
  @eval Symbolics degree(p::Mul, sym=nothing) = sum(degree(k^v, sym) for (k, v) in zip(keys(p.dict), values(p.dict)))
  @btime degree($e)
  # your proposed alternative
  @eval Symbolics degree(p::Mul, sym=nothing) = dot(degree.(keys(p.dict), sym), values(p.dict))
  @btime degree($e)
  # from https://github.com/JuliaSymbolics/Symbolics.jl/pull/677
  @eval Symbolics degree(p::Mul, sym=nothing) = sum(degree(k^v, sym) for (k, v) in p.dict)
  @btime degree($e)
  return
end

main()

#=
  3.432 μs (20 allocations: 760 bytes)  # old
  8.569 μs (60 allocations: 2.40 KiB)   # your implementation
  3.403 μs (16 allocations: 632 bytes)  # https://github.com/JuliaSymbolics/Symbolics.jl/pull/677 

Julia Version 1.7.3
Commit 742b9abb4d* (2022-05-06 12:58 UTC)
Platform Info:
  OS: Linux (x86_64-linux-gnu)
  CPU: Intel(R) Xeon(R) CPU E5-2670 0 @ 2.60GHz
  WORD_SIZE: 64
  LIBM: libm
  LLVM: libLLVM-12.0.1 (ORCJIT, sandybridge)
=#

[bowenszhu]

Oh I see. Broadcast is unnecessary.

Thanks for doing the benchmark!

[bowenszhu]

Could you try the updated one?

degree(p::Mul, sym=nothing) = sum(degree(k, sym) * v for (k, v) in p.dict)

with this input

    @variables xs[1:10000]
    e = prod(x^i for (i, x) in enumerate(xs))

The following measurement

using BenchmarkTools
import SymbolicUtils: Mul
using Symbolics

main() = begin
    @variables xs[1:10000]
    e = prod(x^i for (i, x) in enumerate(xs))
    # old implementation
    @eval Symbolics degree(p::Mul, sym=nothing) = sum(degree(k^v, sym) for (k, v) in zip(keys(p.dict), values(p.dict)))
    @btime degree($e)
    # old proposition with broadcast
    @eval Symbolics degree(p::Mul, sym=nothing) = dot(degree.(keys(p.dict), sym), values(p.dict))
    @btime degree($e)
    # from https://github.com/JuliaSymbolics/Symbolics.jl/pull/677
    @eval Symbolics degree(p::Mul, sym=nothing) = sum(degree(k^v, sym) for (k, v) in p.dict)
    @btime degree($e)
    # updated
    @eval Symbolics degree(p::Mul, sym=nothing) = sum(degree(k, sym) * v for (k, v) in p.dict)
    @btime degree($e)
    return
end

main()

gives

  1.073 ms (39488 allocations: 1.21 MiB)
  800.786 μs (28991 allocations: 609.46 KiB)
  626.015 μs (29488 allocations: 773.23 KiB)
  532.731 μs (19488 allocations: 304.50 KiB)

[t-bltg]

Indeed, this is now even better:

# original benchmark
  3.425 μs (20 allocations: 760 bytes)
  12.404 μs (60 allocations: 2.40 KiB)
  3.305 μs (16 allocations: 632 bytes)
  3.165 μs (12 allocations: 504 bytes)
# n = 1_000
  187.028 μs (3488 allocations: 116.98 KiB)
  135.666 μs (1989 allocations: 47.12 KiB)
  109.584 μs (2488 allocations: 70.11 KiB)
  88.706 μs (1488 allocations: 23.25 KiB)

When updating something for performance, you should never assume, and always benchmark for real concrete improvements.
As a side note, you don't have to feed in large inputs to your benchmarks: the simple expression in #685 (comment) is sufficient to show the issue.

[bowenszhu]

When updating something for performance, you should never assume, and always benchmark for real concrete improvements. As a side note, you don't have to feed in large inputs to your benchmarks: the simple expression in #685 (comment) is sufficient to show the issue.

You're right. Thank you!