Change MutableArithmetics.rewrite to move_factors_into_sums=false #3125

odow · 2022-11-03T22:10:45Z

Needs jump-dev/MutableArithmetics.jl#170

I've been experimenting with a drop-in replacement for MA.@rewrite and MA.rewrite. JuMP throws up some interesting challenges, so it took me a while to get this correct. The tests are now passing with a couple of minimal changes.

A change from compile-time to run-time error for @expression(model, x <= 1)
A correction of the error in @constraint(m, x[i = 1] <= 1) so that it matches the non-macro error obtained from x[i = 1]
A printing change from y * x to x * y. This is just cosmetic, and depends on the order that we encounter the variables, but the new behavior now more closely resembles what the user wrote.

The biggest surprises were some of our design choices:

Rewriting a + b to operate!!(add_mul, a, b) instead of operate!!(+, a, b)
Rewriting a - b to operate!!(sub_mul, a, b) instead of operate!!(-, a, b)
We rewrite only + and - to their mutable add_mul and sub_mul equivalents. We don't rewrite something like operate!!(/, a, b), even if the operation is supported.

Supporting the invalid syntax sum(i+j for i=1:2, j=i:2):

julia> sum(i+j for i=1:2, j=i:2)
ERROR: UndefVarError: i not defined

julia> JuMP.MutableArithmetics2.@rewrite(sum(i+j for i=1:2, j=i:2))
9

The last one took me a while to debug...

There are a couple of options for merging this.

~~keep it in JuMP~~
Add it to MutableArithmetics as a replacement for the existing code
Add it to MutableArithmetics as a new feature, perhaps renaming to rewrite_expr?

codecov · 2022-11-03T22:23:40Z

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.06 🎉

Comparison is base (269742f) 98.05% compared to head (545e139) 98.11%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3125      +/-   ##
==========================================
+ Coverage   98.05%   98.11%   +0.06%     
==========================================
  Files          34       34              
  Lines        4924     4924              
==========================================
+ Hits         4828     4831       +3     
+ Misses         96       93       -3

Impacted Files	Coverage Δ
src/complement.jl	`100.00% <100.00%> (ø)`
src/macros.jl	`98.63% <100.00%> (ø)`

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

odow · 2022-11-17T02:44:25Z

Urgh. We're back to the Union issue in MA, this time in another place.

julia> import MutableArithmetics

julia> const MA = MutableArithmetics
MutableArithmetics

julia> MA.operate(*, [1, 2]', Vector{Union{String,Float64}}([1.0, 2.0]))
5.0

julia> MA.operate(*, [1 2; 3 4], Vector{Union{String,Float64}}([1.0, 2.0]))
ERROR: MethodError: no method matching zero(::Type{Union{Float64, String}})
Closest candidates are:
  zero(::Union{Type{P}, P}) where P<:Dates.Period at /Users/julia/buildbot/worker/package_macos64/build/usr/share/julia/stdlib/v1.6/Dates/src/periods.jl:53
  zero(::MutableArithmetics.AbstractMutable) at /Users/oscar/.julia/dev/MutableArithmetics/src/dispatch.jl:784
  zero(::SparseArrays.AbstractSparseArray) at /Users/julia/buildbot/worker/package_macos64/build/usr/share/julia/stdlib/v1.6/SparseArrays/src/SparseArrays.jl:55
  ...
Stacktrace:
 [1] _instantiate_zero(#unused#::Type{Union{Float64, String}})
   @ MutableArithmetics ~/.julia/dev/MutableArithmetics/src/interface.jl:28
 [2] promote_operation_fallback(op::typeof(*), #unused#::Type{Int64}, #unused#::Type{Union{Float64, String}})
   @ MutableArithmetics ~/.julia/dev/MutableArithmetics/src/interface.jl:51
 [3] promote_operation(::typeof(*), ::Type, ::Type)
   @ MutableArithmetics ~/.julia/dev/MutableArithmetics/src/interface.jl:113
 [4] promote_sum_mul(T::Type, S::Type)
   @ MutableArithmetics ~/.julia/dev/MutableArithmetics/src/implementations/LinearAlgebra.jl:186
 [5] promote_array_mul(#unused#::Type{Matrix{Int64}}, #unused#::Type{Vector{Union{Float64, String}}})
   @ MutableArithmetics ~/.julia/dev/MutableArithmetics/src/implementations/LinearAlgebra.jl:191
 [6] operate(#unused#::typeof(*), A::Matrix{Int64}, B::Vector{Union{Float64, String}})
   @ MutableArithmetics ~/.julia/dev/MutableArithmetics/src/implementations/LinearAlgebra.jl:388
 [7] top-level scope
   @ REPL[5]:1

.github/workflows/aqua.yml

test/runtests.jl

docs/make.jl

odow · 2022-12-08T19:47:47Z

Run the larger JuMP benchmark suite

odow · 2022-12-09T02:00:57Z

I ran the benchmarks here: https://github.com/jump-dev/JuMP.jl/blob/master/test/perf/JuMPBenchmarks.jl.

No major changes.

The linear examples use a lot more memory, but are 30% faster.
The quadratic summations are slightly slower.
Everything else is invariant.

The linear examples use more memory because we're no longer collapsing sums into each other. If you write

model = Model()
@variable(model, x[1:3])
@expression(model, sum(x[i] for i in 1:3) + sum(x[i] for i in 1:3))

the old MA produced

quote
    var"#11###286" = (JuMP.MutableArithmetics).Zero()
    for i = 1:3
        var"#11###286" = (JuMP.MutableArithmetics).operate!!((JuMP.MutableArithmetics).add_mul, var"#11###286", x[i])
    end
    var"#12###285" = var"#11###286"
    for i = 1:3
        var"#12###285" = (JuMP.MutableArithmetics).operate!!((JuMP.MutableArithmetics).add_mul, var"#12###285", x[i])
    end
    var"#13###284" = var"#12###285"
end

whereas now we do

quote
    var"#11###284" = MutableArithmetics.Zero()
    for i = 1:3
        var"#11###284" = (MutableArithmetics.operate!!)(MutableArithmetics.add_mul, var"#11###284", x[i])
    end
    var"#12###285" = MutableArithmetics.Zero()
    for i = 1:3
        var"#12###285" = (MutableArithmetics.operate!!)(MutableArithmetics.add_mul, var"#12###285", x[i])
    end
    var"#13###286" = (MutableArithmetics.operate!!)(MutableArithmetics.add_mul, var"#11###284", var"#12###285")
end

The new rewrite follows the syntax (there are two sums), at the cost of creating an additional AffExpr object.
The same thing happens if you have nested sums like sum(j * sum(x[i] for i in 1:N) for j in 1:N) (which is what the benchmarks are testing).

Here's the output of the benchmarks:

(base) oscar@Oscars-MBP JuMP % git checkout master
Switched to branch 'master'
Your branch is up to date with 'master'.
(base) oscar@Oscars-MBP JuMP % julia --project=. test/perf/JuMPBenchmarks.jl -f expr.txt
... lines omitted ...
(base) oscar@Oscars-MBP JuMP % git checkout od/ma2
Switched to branch 'od/ma2'
Your branch is up to date with 'origin/od/ma2'.
(base) oscar@Oscars-MBP JuMP % julia --project=. test/perf/JuMPBenchmarks.jl -f expr.txt --compare

========== Results ==========

bench_macro_linear_010
BenchmarkTools.TrialJudgement: 
  time:   -31.57% => improvement (5.00% tolerance)
  memory: +191.15% => regression (1.00% tolerance)

bench_dense_axis_constraints_index
BenchmarkTools.TrialJudgement: 
  time:   -0.68% => invariant (5.00% tolerance)
  memory: +0.00% => invariant (1.00% tolerance)

bench_array_sum_050
BenchmarkTools.TrialJudgement: 
  time:   -2.69% => invariant (5.00% tolerance)
  memory: +0.00% => invariant (1.00% tolerance)

bench_vector_sum_050
BenchmarkTools.TrialJudgement: 
  time:   -0.01% => invariant (5.00% tolerance)
  memory: +0.00% => invariant (1.00% tolerance)

bench_sparse_axis_constraints_iterate
BenchmarkTools.TrialJudgement: 
  time:   -4.00% => invariant (5.00% tolerance)
  memory: +0.00% => invariant (1.00% tolerance)

bench_matrix_quadratic_product_010
BenchmarkTools.TrialJudgement: 
  time:   -1.49% => invariant (5.00% tolerance)
  memory: +0.00% => invariant (1.00% tolerance)

bench_matrix_affine_product_010
BenchmarkTools.TrialJudgement: 
  time:   -0.45% => invariant (5.00% tolerance)
  memory: +0.00% => invariant (1.00% tolerance)

bench_sparse_axis_constraints_index
BenchmarkTools.TrialJudgement: 
  time:   +2.37% => invariant (5.00% tolerance)
  memory: +0.00% => invariant (1.00% tolerance)

bench_vector_sum_010
BenchmarkTools.TrialJudgement: 
  time:   -0.43% => invariant (5.00% tolerance)
  memory: +0.00% => invariant (1.00% tolerance)

bench_dense_axis_constraints_iterate
BenchmarkTools.TrialJudgement: 
  time:   -0.04% => invariant (5.00% tolerance)
  memory: +0.00% => invariant (1.00% tolerance)

bench_matrix_sum_050
BenchmarkTools.TrialJudgement: 
  time:   +4.17% => invariant (5.00% tolerance)
  memory: +0.00% => invariant (1.00% tolerance)

bench_macro_quad_050
BenchmarkTools.TrialJudgement: 
  time:   +12.98% => regression (5.00% tolerance)
  memory: -15.85% => improvement (1.00% tolerance)

bench_matrix_affine_product_050
BenchmarkTools.TrialJudgement: 
  time:   -1.21% => invariant (5.00% tolerance)
  memory: +0.00% => invariant (1.00% tolerance)

bench_matrix_quadratic_product_050
BenchmarkTools.TrialJudgement: 
  time:   +0.96% => invariant (5.00% tolerance)
  memory: +0.00% => invariant (1.00% tolerance)

bench_macro_linear_050
BenchmarkTools.TrialJudgement: 
  time:   -39.13% => improvement (5.00% tolerance)
  memory: +341.27% => regression (1.00% tolerance)

bench_array_sum_010
BenchmarkTools.TrialJudgement: 
  time:   -2.52% => invariant (5.00% tolerance)
  memory: +0.00% => invariant (1.00% tolerance)

bench_matrix_sum_010
BenchmarkTools.TrialJudgement: 
  time:   -1.35% => invariant (5.00% tolerance)
  memory: +0.00% => invariant (1.00% tolerance)

bench_macro_quad_010
BenchmarkTools.TrialJudgement: 
  time:   +5.64% => regression (5.00% tolerance)
  memory: +7.79% => regression (1.00% tolerance)

odow · 2023-06-08T22:44:50Z

Running extension-tests on this: https://github.com/jump-dev/JuMP.jl/actions/runs/5216388377

Now that I've used the nonlinear branch a bunch, I'm pretty confident that this change is both safe to do. I'd like to get it in separately to the nonlinear PR so we're not changing too much in a single go.

odow · 2023-06-08T23:01:32Z

@pulsipher this breaks the tests of InfininteOpt: https://github.com/jump-dev/JuMP.jl/actions/runs/5216388377/jobs/9415059599

It's the same set of changes here:

https://github.com/infiniteopt/InfiniteOpt.jl/pull/308/files#diff-6f28d0077bbc402cee4e236a0ca51e04d420de58e2a9c5897868ece68f2de188L82

We now build expressions that match what you type, instead of these somewhat arbitrary x / y --> x * (1 / y) reformulations.

Thoughts on merging this separately to the nonlinear stuff?

odow · 2023-06-08T23:02:40Z

This also seemed to fix a series of test failures I'd seen on Alpine, likely because we're passing more sensible formulations to the solver.

pulsipher · 2023-06-08T23:17:55Z

@pulsipher this breaks the tests of InfininteOpt: https://github.com/jump-dev/JuMP.jl/actions/runs/5216388377/jobs/9415059599
It's the same set of changes here:
https://github.com/infiniteopt/InfiniteOpt.jl/pull/308/files#diff-6f28d0077bbc402cee4e236a0ca51e04d420de58e2a9c5897868ece68f2de188L82

We now build expressions that match what you type, instead of these somewhat arbitrary x / y --> x * (1 / y) reformulations.

Thoughts on merging this separately to the nonlinear stuff?

I am definitely in favor of this change to MutableArithmetics. Naturally, it would be easiest for me to have this merged at about the same time as #3106, but it shouldn't be that much work to breakup my existing pull request in InfiniteOpt accordingly. So, I guess in short I have no problem with this being merged.

odow · 2023-06-08T23:19:36Z

If it didn't break your tests I'd merge this ASAP, but perhaps it's okay as a PR immediately before #3106, so that they both come out in the same JuMP release.

odow · 2023-06-08T23:20:51Z

I guess the other point to readers is that this isn't a breaking change per se. It just breaks the tests in which @pulsipher compares two expressions; the expressions are equivalent mathematically, but they have a different structure in code.

odow · 2023-06-19T04:05:27Z

Closing for now. The od/nlp-expr does it a different way, and perhaps it's okay to merge as one.

odow force-pushed the od/ma2 branch from 8bd20e0 to f2a8afa Compare November 17, 2022 02:20

odow mentioned this pull request Nov 17, 2022

[Nonlinear] add new nonlinear interface #3106

Merged

4 tasks

odow commented Nov 21, 2022

View reviewed changes

.github/workflows/aqua.yml Outdated Show resolved Hide resolved

odow commented Nov 21, 2022

View reviewed changes

test/runtests.jl Outdated Show resolved Hide resolved

odow commented Nov 21, 2022

View reviewed changes

docs/make.jl Outdated Show resolved Hide resolved

odow force-pushed the od/ma2 branch from e90afab to 279887c Compare November 21, 2022 23:03

odow mentioned this pull request Nov 22, 2022

Prep for v1.1.0 jump-dev/MutableArithmetics.jl#183

Merged

1 task

odow force-pushed the od/ma2 branch 2 times, most recently from 61f4000 to 8310930 Compare November 23, 2022 05:58

odow changed the title ~~WIP: refactor and simplify MA.rewrite~~ Change MutableArithmetics.rewrite to move_factors_into_sums=false Nov 23, 2022

odow force-pushed the od/ma2 branch from 8310930 to f894f1c Compare December 7, 2022 00:27

odow force-pushed the od/ma2 branch from f894f1c to c16e70a Compare December 9, 2022 01:27

odow mentioned this pull request Jan 23, 2023

Precompilation in the SnoopPrecompile/Julia 1.9+ world jump-dev/MathOptInterface.jl#2226

Closed

odow force-pushed the od/ma2 branch from b94d17e to 03bf626 Compare February 22, 2023 00:03

odow mentioned this pull request Mar 22, 2023

Expression performance when ; init = 0 used in sum jump-dev/MutableArithmetics.jl#204

Closed

odow force-pushed the od/ma2 branch from 03bf626 to e7e5bad Compare March 22, 2023 03:36

odow added Status: Needs review Status: Needs developer call This should be discussed on a monthly developer call and removed Status: Needs review labels Mar 22, 2023

odow removed the Status: Needs developer call This should be discussed on a monthly developer call label Jun 2, 2023

Change MutableArithmetics.rewrite to move_factors_into_sums=false

5fdad52

odow force-pushed the od/ma2 branch from e7e5bad to 5fdad52 Compare June 8, 2023 04:57

Merge branch 'master' into od/ma2

545e139

odow closed this Jun 19, 2023

odow deleted the od/ma2 branch June 19, 2023 04:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change MutableArithmetics.rewrite to move_factors_into_sums=false #3125

Change MutableArithmetics.rewrite to move_factors_into_sums=false #3125

odow commented Nov 3, 2022 •

edited

Loading

codecov bot commented Nov 3, 2022 •

edited

Loading

odow commented Nov 17, 2022

odow commented Dec 8, 2022

odow commented Dec 9, 2022

odow commented Jun 8, 2023

odow commented Jun 8, 2023

odow commented Jun 8, 2023

pulsipher commented Jun 8, 2023

odow commented Jun 8, 2023

odow commented Jun 8, 2023

odow commented Jun 19, 2023

Change MutableArithmetics.rewrite to move_factors_into_sums=false #3125

Change MutableArithmetics.rewrite to move_factors_into_sums=false #3125

Conversation

odow commented Nov 3, 2022 • edited Loading

codecov bot commented Nov 3, 2022 • edited Loading

Codecov Report

odow commented Nov 17, 2022

odow commented Dec 8, 2022

odow commented Dec 9, 2022

odow commented Jun 8, 2023

odow commented Jun 8, 2023

odow commented Jun 8, 2023

pulsipher commented Jun 8, 2023

odow commented Jun 8, 2023

odow commented Jun 8, 2023

odow commented Jun 19, 2023

odow commented Nov 3, 2022 •

edited

Loading

codecov bot commented Nov 3, 2022 •

edited

Loading