-
-
Notifications
You must be signed in to change notification settings - Fork 394
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Replace parse_expr and destructive_add by MutableArithmetics #2107
Conversation
ea093fc
to
2a7f27a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice to see all the deleted code! Here's a superficial review.
Codecov Report
@@ Coverage Diff @@
## master #2107 +/- ##
==========================================
+ Coverage 91.08% 91.33% +0.25%
==========================================
Files 41 41
Lines 4376 4053 -323
==========================================
- Hits 3986 3702 -284
+ Misses 390 351 -39
Continue to review full report at Codecov.
|
b9821f9
to
71734dd
Compare
44daf33
to
a1577a8
Compare
4616361
to
1fc5b0e
Compare
Ready for final review :) |
@@ -11,6 +11,10 @@ function _let_code_block(ex::Expr) | |||
return ex.args[2] | |||
end | |||
|
|||
function _error_curly(x) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this in parse_nlp
? We should also throw this error for the regular macros.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is now called inside MutableArithmetics for the regular macro.
BREAKING CHANGES
@SDconstraint(model, A >= 1)
throwsERROR: Operation
+between
Array{VariableRef,2}and
Int64is not allowed. You should use broadcast.
instead of broadcasting,@SDconstraint(model, A >= 0)
is equivalent to@contsraint(model, A in PSDCone())
JuMP implements an arithmetic between the following types:
Number
s (that are eventually converted toFloat64
)AbstractVariableRef
GenericAffExpr
GenericQuadExpr
.This arithmetic has two key features:
GenericAffExpr
andGenericQuadExpr
are mutable objects and creating new ones is costly.The consequence of 1) is that many
Base
functions do not work out of the box (see e.g. JuliaLang/julia#26344 and JuliaLang/julia#26344).The reason is that these are usually tested with
Float64
types which are stable under multiplications, addition, ...The common mistakes are
VariableRef
and aFloat64
, because of the promotion we will get atQuadExpr
as result. This is the case in many places inSparseArrays
.For these reasons, many Base functions need to be rewritten for JuMP types to make them work.
The consequence of 2) is that even if generic functions are implemented correctly, they cannot exploit the mutability of the JuMP expression without have JuMP as a dependency. This is obviously not possible for
Base
but it may also be cumbersome in other cases (e.g. MultivariatePolynomials supports any coefficient type and don't want to have JuMP as a dependency just because the coefficients could be JuMP expressions).JuMP contains code to:
Base
function for JuMP types and make them efficient (addresses 1) and 2)).This code is quite involved and could benefit other mutable types as well (e.g.
BigInt
, polynomials, MOI functions, ...).Just like MOI serves an an interface between solvers packages and packages writing optimization models, MutableArithmetics (MA) defines a mutable arithmetics API which allows packages defining mutable types and packages writing generic functions working on arbitrary arithmetics to work together (efficiently, exploiting the mutability of the types) without having to know about each other.
In this PR,
parse_expr
is replaced byMA.rewrite
anddestructive_add
is replaced byMA.add_mul!
. The next step are to move the array-related code ofsrc/operators.jl
to MA and check that there is no performance regression.src/operators.jl
to MA.Benchmarks
Here are the results of the benchmarks of
test/perf
with Julia v1.3.0.test/perf/macros.jl
Before the PR:
After the PR:
test/perf/speed.jl
Before the PR:
After the PR:
test/perf/matrix_product.jl
Before the PR:
After the PR:
test/perf/vector_speedtest.jl
Before the PR:
After the PR:
Closes #2005
Closes #2039
Closes #2102
Closes #2106
Closes #2125