Should we define zero(AffExpr) and one(AffExpr)?

[mlubin]

The implementation of zeros(AffExpr, n) looks like fill!(Array{AffExpr}(n), zero(AffExpr)), which is problematic because the result is n entries that refer to the same AffExpr (#1113). Same for ones.

This leads me to question if we should define zero(AffExpr) or one(AffExpr) at all given that code may be written assuming that these return immutable objects. The replacement is AffExpr(0.0) and AffExpr(1.0).

@daschw

[blegat]

If Base is doing this, they may be implicitly assuming that only immutable objects should define zero and one.

Maybe we could have two version of AffExpr ? One immutable and one mutable.
The mutability of AffExpr is only used inside JuMP macros so we could take care of using the mutable version there.
If the user do operation on variables outside JuMP macros it will become the immutable version.

[mlubin]

The mutability of AffExpr is only used inside JuMP macros so we could take care of using the mutable version there.

Not true, we also expose push! and some other functions to append to AffExprs. These are pretty useful in performance-critical situations.

[joaquimg]

The replacement is AffExpr(0.0) and AffExpr(1.0)

seems reasonable.

[blegat]

Not true, we also expose push! and some other functions to append to AffExprs. These are pretty useful in performance-critical situations.

We could have a warning push! not supported in AffExpr, please convert it to a MutableAffExpr.

[mlubin]

Having both a mutable and immutable AffExpr is worth considering, but I'd put it off until after 0.19.

[joaquimg]

What is the advantage of having the immutable version?

[mlubin]

This example by @jdlara-berkeley suggests a similar issue with convert:

m = Model()
x = @variable(m, x >= 0)
NetInjectionVar =  Array{AffExpr,2}(undef, 5, 24)
NetInjectionVar[:] .= 0.0
JuMP.add_to_expression!(NetInjectionVar[1,1],x)
NetInjectionVar
5×24 Array{JuMP.GenericAffExpr{Float64,JuMP.VariableRef},2}:
 x  x  x  x  x  x  x  x  x  x  x  x  x  x  x  x  x  x  x  x  x  x  x  x
 x  x  x  x  x  x  x  x  x  x  x  x  x  x  x  x  x  x  x  x  x  x  x  x
 x  x  x  x  x  x  x  x  x  x  x  x  x  x  x  x  x  x  x  x  x  x  x  x
 x  x  x  x  x  x  x  x  x  x  x  x  x  x  x  x  x  x  x  x  x  x  x  x
 x  x  x  x  x  x  x  x  x  x  x  x  x  x  x  x  x  x  x  x  x  x  x  x

I claim that this would be less of a gotcha if you had to write

NetInjectionVar[:] .= AffExpr(0.0)

instead of

NetInjectionVar[:] .= 0.0

[odow]

Now that we've changed to Dicts, is this still relevant?

m = Model()
x = @variable(m, x >= 0)
NetInjectionVar =  Array{JuMP.AffExpr,2}(undef, 5, 24)
NetInjectionVar[:] .= zero(AffExpr)
NetInjectionVar[1,1] += x

julia> NetInjectionVar
5×24 Array{JuMP.GenericAffExpr{Float64,VariableRef},2}:
 x  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0
 0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0
 0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0
 0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0
 0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0

[mlubin]

I just updated my example. The issue still exists on Julia 1.0 and JuMP/MOI.

[odow]

We should come back to this, since removing it is going to be breaking for JuMP 1.0.

Another option is

mutable struct GenericAffExpr{V,K}
    mutable::Bool
    terms::OrderedDict{K,V}
    constant::V
end

function GenericAffExpr{K,V}(terms::OrderedDict{K,V}, constant::V) where {K,V}
    return GenericAffExpr(true, terms, constant)
end

function Base.zero(::Type{GenericAffExpr{K,V}}) where {K,V}
    return GenericAffExpr(false, OrderedDict{K,V}(), 0.0)
end

function Base.one(::Type{GenericAffExpr{K,V}}) where {K,V}
    return GenericAffExpr(false, OrderedDict{K,V}(), 1.0)
end

function add_to_expression!(aff::GenericAffExpr)
    if aff.mutable
        # ...
    else
        # ...
    end
end

In almost all cases the object is mutable (and Julia is probably smart enough to optimize away the if). The only exceptions are zero and one.

This is also breaking because would be people would have to write the following if they were unsure if NetInjectionVar was mutable.

NetInjectionVar[1,1] = JuMP.add_to_expression!(NetInjectionVar[1,1],x)

Having a separate type seems problematic for type stability reasons. (Users will say x .= AffExpr(0.0) is fast but x .= 0 is slow?)

[blegat]

Given that the issue only happens if the user calls JuMP.add_to_expression! explicitely, I don't think that it's an issue. The users using this function should be aware that it's mutable operations and that it would modify the object itself so if it's referenced from several place then it will affect all of them. We cannot prevent the user to have multiple references of a same JuMP expression and mutate it.

[lkapelevich]

+1 for the original suggestion to remove zero(AffExpr) and one(AffExpr)

The users using this function should be aware that it's mutable

this is pretty clear

so if it's referenced from several place then it will affect all of them

...but I didn't think about how Base.zeros and Base.ones work on mutable objects, so calling zeros still gave me an unexpected result.

I think leaving things as is is also understandable, but defining an immutable GenericAffExpr would be more confusing as it creates a separate gotcha (that the AffExprs from zeros/ones are mutable).

[odow]

To summarize, we can either:

• Remove zero and one and make expressions much harder to work with and break a lot of code that uses LinearAlgebra
• Keep zero and one and cause subtle bugs if people are creating arrays of expressions and then modifying them in-place

My conclusion from #2711 is the second option. The first one is going to cause problems for new users and we will have the forum flooded with questions. The second is going to cause problems for more advanced users, who should, by then, have an understanding of why it happens with sufficient documentation.

[jd-lara]

@odow FWIW removing the zeros of AffnExpr makes it harder to compose expressions before they get used in building the constraint. We use Zeros of AffnExpr a lot in PowerSimulations.jl to add programmatically the devices. Probably there will be workarounds but it will be less clean than it currently is.

Edit: Lol, I just realized that I had added an example back in 2018 when PowerSimulations.jl started.

[odow]

You fall into the second category. If needed, you should use AffExpr(0.0) to avoid accidentally filling a matrix with a single instance of AffExpr.

[blegat]

I agree with the second option.