Fix #889

[blegat]

No description provided.

[mlubin]

Thanks for the PR! Should we be doing this though or should we move towards the built-in broadcast syntax?

[blegat]

The issue with the broadcast syntax is that it does not preserve the sparsity

julia> f(x) = x*x
f (generic function with 1 method)

julia> X = sparse([1, 3], [2, 3], [3, 9])
3×3 sparse matrix with 2 Int64 nonzero entries:
    [1, 2]  =  3
    [3, 3]  =  9

julia> f.(X)
3×3 Array{Int64,2}:
 0  9   0
 0  0   0
 0  0  81

julia> broadcast(f, X)
3×3 Array{Int64,2}:
 0  9   0
 0  0   0
 0  0  81

[mlubin]

Is there an upstream discussion on this?

[blegat]

The thing is that broadcast needs to be sure that the function maps zero to zero.
Currently there is the _broadcast_unary_nz2nz_z2z where we ensure that nonzero are mapped to nonzero and zero are mapped to zeros so that it can just apply the function to every nonzero entry.
There is also this pull request: JuliaLang/julia#19239 that checks when you call broadcast whether the function maps zero to zero.

Do you think we should only support getvalue of objects created by JuMP (like @variable [1:n,1:m]) and ask the user to use getvalue. when he has created a container of JuMP expressions since it seems that in Julia 0.6, broadcast on sparse matrices will return sparse matrices ?

[mlubin]

Yes, I'd say we shouldn't implement any magic for types that aren't directly created by JuMP. JuMP will never directly create a sparse matrix of variables. Mapping zero to zero is nonsensical in this case though, hope the broadcasting works when you have nonnumeric entries.

[blegat]

If the JuliaLang pull request is merged as is, it will fail since it will do

julia> getvalue(typeof(zero(JuMP.Variable))) == zero(Variable)
ERROR: MethodError: no method matching getvalue(::Type{JuMP.GenericAffExpr{Float64,JuMP.Variable}})
Closest candidates are:
  getvalue(::JuMP.NonlinearExpression) at /home/blegat/.julia/v0.5/JuMP/src/nlp.jl:1236
  getvalue(::JuMP.NonlinearParameter) at /home/blegat/.julia/v0.5/JuMP/src/nlp.jl:39
  getvalue(::JuMP.GenericQuadExpr{Float64,JuMP.Variable}) at /home/blegat/.julia/v0.5/JuMP/src/quadexpr.jl:92
  ...

[mlubin]

Well that's an issue. Should we let them know?

[blegat]

We could also define getvalue for AffineExpr

[mlubin]

IIRC that should already be defined.

[blegat]

Oh I didn't write it correctly... What is going to be called is

julia> getvalue(zero(Variable)) == zero(Variable)
false
julia> typeof(zero(Variable))
JuMP.GenericAffExpr{Float64,JuMP.Variable}
julia> typeof(getvalue(zero(Variable)))
Float64

The problem is that there is no method == between an AffExpr and something else. If we add

(==)(lhs::AffExpr,rhs) = (sum(abs(lhs.coeffs)) == 0) && (lhs.constant == rhs)
(==)(lhs,rhs::AffExpr) = rhs == lhs

then we get

julia> getvalue(zero(Variable)) == zero(Variable)
true

[joehuchette]

Any reason not to merge this?

[mlubin]

I remove my objection. If you update the PR and get tests passing this is okay to merge. We'll revisit the use of broadcast syntax for everything in JuMP at a later point.

[codecov-io]

Current coverage is 88.30% (diff: 72.00%)

Merging #891 into master will increase coverage by 0.43%

@@             master       #891   diff @@
==========================================
  Files            18         18          
  Lines          4403       4687   +284   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           3869       4139   +270   
- Misses          534        548    +14   
  Partials          0          0          

Powered by Codecov. Last update ff2e4ce...03e0284

[mlubin]

Isn't getvalue(::JuMPContainer) already explicitly defined somewhere else? There's a reason it's not on this list.

[blegat]

That is an excellent question. I have updated the PR with a proposal to unify these accessors on JuMPContainer/AbstractArray

[joehuchette]

Seems reasonable to me, but Travis has some pretty crazy-looking errors.

[blegat]

Wouldn't it be more consistent with getvalue if getdual outputs a warning and a vector of NaN instead of throwing an error ?

[joehuchette]

I would think so, yes.  On Wed, Dec 14, 2016 at 12:47 PM -0500, "Benoît Legat" <notifications@github.com> wrote: Wouldn't it be more consistent with getvalue if getdual outputs a warning and a vector of NaN instead of throwing an error ? — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

[mlubin]

@blegat, if you tidy this up this week we can get it in for 0.15.

[blegat]

@mlubin @joehuchette The PR is ready for review, the tests are passing for Julia v0.5

[joehuchette]

Thanks, @blegat!