Skip to content
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

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

Closed
blegat opened this issue Jan 18, 2019 · 7 comments
Closed

Comments

@blegat
Copy link
Member

blegat commented Jan 18, 2019

See #634 (comment)

@blegat blegat added this to the v0.8.2 milestone Jan 18, 2019
@mlubin
Copy link
Member

mlubin commented Jan 18, 2019

No, we shouldn't. This is a mistake in JuMP that can lead to hard to find bugs (jump-dev/JuMP.jl#1151 (comment)).

@blegat
Copy link
Member Author

blegat commented Jan 18, 2019

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
Copy link
Member

mlubin commented Jan 18, 2019

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
Copy link
Member Author

blegat commented Jan 19, 2019

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:
JuliaLang/julia#29168 (comment)
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
Copy link
Member

mlubin commented Jan 20, 2019

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
Copy link
Member

odow commented Mar 8, 2021

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

@odow
Copy link
Member

odow commented May 18, 2021

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

@odow odow closed this as completed May 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants