jump-dev / MathOptInterface.jl

A data structure for mathematical optimization problems
http://jump.dev/MathOptInterface.jl/
Other
392 stars 87 forks source link

Should we extend Base.zero for ScalarAffineFunction and ScalarQuadraticFunction #636

Closed blegat closed 3 years ago

blegat commented 5 years ago

See https://github.com/JuliaOpt/MathOptInterface.jl/pull/634#issuecomment-455572745

mlubin commented 5 years ago

No, we shouldn't. This is a mistake in JuMP that can lead to hard to find bugs (https://github.com/JuliaOpt/JuMP.jl/issues/1151#issuecomment-402214011).

blegat commented 5 years ago

I would argue that the case is a bit different from JuMP where JuMP expressions are meant to be userfriendly and the user should be able to manipulate just like numbers. On the other hand, MOI function utilities are primarily used by bridges and we may expect more from the user (and some say that zeros should be thought as a short-hand for fill hence it's ok to define zero for mutable struct). We could also overload zeros on MOI functions and throw an error suggesting to use fill instead (as zeros(::{ScalarAffineFunction{T}}, n) may seem ambiguous on whether the user is trying to build a VectorAffineFunction).

The reason I would like zero to be defined is that it allows MOI functions to be used in a code that doesn't know anything about MOI and simply uses standard arithmetic operations. This is needed for SumOfSquares which relies on MultivariatePolynomials which does not assumes anything about the coefficient type. The coefficients used to be JuMP expressions but now with the bridges, it will be MOI functions.

mlubin commented 5 years ago

The reason I would like zero to be defined is that it allows MOI functions to be used in a code that doesn't know anything about MOI and simply uses standard arithmetic operations.

That's exactly the issue. Code that doesn't know anything about MOI probably wasn't written with mutable number types in mind and isn't tested with them. We shouldn't implement the zero method if we violate the assumptions of the interface. This is about correctness and avoiding traps, which applies regardless of how advanced the users are. If MultivariatePolynomials is aware of mutable zeros, it can define and use maybe_mutable_zero(T) that is extensible by callers.

I don't know if implementing zeros is sufficient. We don't know where else this assumption is used.

blegat commented 5 years ago

That's exactly the issue. Code that doesn't know anything about MOI probably wasn't written with mutable number types in mind and isn't tested with them.

If the code is written with only generic operations, it won't do any mutated operation so there won't be any issue. The issue arises if the user uses zeros and then call JuMP.destructive_add! or MOIU.operate!. For instance, with BigInt, if the user calls zeros and then calls the mutated operations, it will also be an issue but in Base, it is not considered an issue as if mutated operations are used then the code is specific to BigInt so the user should know that by calling zeros, it fills it the the same BigInt. Therefore for a performance reason they prefer zeros to put the same BigInt at each entry since it will only be used on generic code that don't mutate hence it will be equivalent and more efficient to use the same instance for each entry: https://github.com/JuliaLang/julia/issues/29168#issuecomment-421053888 The same reasoning applies for MOI and JuMP IMO. If they find it ok for BigInt it is ok for JuMP and MOI too.

If MultivariatePolynomials is aware of mutable zeros, it can define and use maybe_mutable_zero(T) that is extensible by callers.

That would require MOI to have MultivariatePolynomials in its dependencies which is weird. In fact, the lack of generic mutated arithmetic has been worrying me for quite some time. Because of this, MultivariatePolynomials isn't as efficient as it could be for BigInt, JuMP expressions and MOI functions. In the long term, there should be a MutatedArithmetics packages that defined mutable arithmetics that fallbacks to non-mutable (a bit like MOIU.operate!) and defines it for BigInt. JuMP and MOI could implements the API of this package to allow generic code to be made efficient with BigInt, JuMP and MOI types without being specifically written for them.

mlubin commented 5 years ago

Ok, since this shouldn't cause issues with code that doesn't try to mutate the numbers, let's drop this from the 0.8.2 milestone.

That would require MOI to have MultivariatePolynomials in its dependencies which is weird.

Not necessarily. Couldn't SumOfSquares implement the mutating interface for MOI when it calls MultivariatePolynomials? It might need a wrapper type around MOI to avoid type piracy.

odow commented 3 years ago

Can this be closed? We seem to be getting along fine without defining it...

odow commented 3 years ago

Closing. We can re-open in future if we ever have a good reason for it.