clean up some linear algebra primitives for JuMP eltypes #1456

jrevels · 2018-09-05T20:08:57Z

Did this while working on some of the TODOs floating about this code (e.g. properly intercepting sparse matmul on Julia >v0.7). I can just continue that work within this PR, or open a different PR on top of this one later - whatever's easiest for review.

Let's see if tests pass with what I currently have...

EDIT: Tests now pass locally

mlubin

Please fix the CI failure on 0.6.

mlubin · 2018-09-06T01:51:22Z

src/operators.jl

+
+densify_with_jump_eltype(x::AbstractMatrix) = convert(Matrix, x)
+
+function densify_with_jump_eltype(x::SparseMatrixCSC{V}) where V<:AbstractVariableRef


Why have special behavior for SparseMatrixCSC{<:AbstractVariableRef} and not AbstractMatrix{<:AbstractVariableRef}?

Just because (AFAICT, maybe I missed something) that was what the code was doing before, when it was pirate-overloading Matrix. I could change to AbstractMatrix{<:AbstractVariableRef} if you want.

mlubin · 2018-09-06T01:52:26Z

src/operators.jl

+    return convert(Matrix{GenericAffExpr{Float64,V}}, x)
+end
+
+# see https://github.com/JuliaLang/julia/pull/18218


Nit: Proper capitalization and punctuation in comments. "See JuliaLang/julia#18218."

mlubin · 2018-09-06T01:52:46Z

src/operators.jl

+_A_mul_B_ret_dims(A::AbstractMatrix, B::AbstractVector) = (size(A, 1),)
+_A_mul_B_ret_dims(A::AbstractMatrix, B::AbstractMatrix) = (size(A, 1), size(B, 2))
+
+# don't do size checks here; defer that to `_A_mul_B(A, B)`


Nit: Proper capitalization and punctuation in comments.

mlubin · 2018-09-06T01:54:13Z

src/operators.jl

-        _sizehint_expr!(q, n)
-        for k ∈ 1:n
-            tmp = convert(T, lhs[k,i]*rhs[k,j]) # transpose
+function fillwithzeros!(A, ::Type{T}) where {T}


Style nit: fill_with_zeros! or fill_with_zeros.

mlubin · 2018-09-06T01:55:39Z

src/operators.jl

+end
+
+###############################################################################
+# `_A_mul_B!(ret, A, B)` adds the result of `A*B` into the buffer `ret`. We roll our own


Nit: It's weird for the ############################################################################### separator not to cover the width of the text. Could we remove these? (I know they're used above, but they're pretty old..)

Sure, just used them since they already seemed to be here.

Is there a preferred "code block comment separator" format for JuMP? I don't necessarily care what format it takes, but breaking the code up with these kind of things vastly improves readability.

"code block comment separator" format for JuMP?

No, not really. I thought about it briefly but don't have a clear answer. Maybe more consistent use of empty lines would make it easier to separate sections without large horizontal comment bars.

mlubin · 2018-09-06T02:05:02Z

src/operators.jl

 function Base.:-(x::AbstractArray{T}) where T<:JuMPTypes
    ret = similar(x, typeof(-one(T)))
    for I in eachindex(ret)
        ret[I] = -x[I]
    end
    ret
 end
-Base.:*(x::AbstractArray{T}) where {T<:JuMPTypes} = x
+
+# TODO This will interact poorly with other packages that overload broadcast on Julia >v0.7;


What breaks if we don't define this code on 0.7+? It's not clear why we need to override this.

good point, I'll try wrapping this in a VERSION conditional

mlubin · 2018-09-06T02:05:45Z

src/operators.jl

+
+Base.:*(x::AbstractArray{T}) where {T<:JuMPTypes} = x
+
+Base.:/(A::SparseMatrixCSC{T}, B::Number) where {T<:JuMPTypes} = SparseMatrixCSC(A.m, A.n, copy(A.colptr), copy(A.rowval), A.nzval ./ B)


Nit: Use one-line functions only when the function fits on one line (i.e., 80 characters).

mlubin · 2018-09-06T02:07:38Z

src/operators.jl

 end

+Base.:*(A::Number, B::SparseMatrixCSC{T}) where {T<:JuMPTypes} = SparseMatrixCSC(B.m, B.n, copy(B.colptr), copy(B.rowval), A .* B.nzval)


Why aren't these covered by the built-ins? Because JuMPTypes aren't Numbers? This merits a comment.

That's a good question. This code was here before I got here, though :p I'll try to read deeper and figure out an answer

src/operators.jl

mlubin · 2018-09-06T02:13:26Z

src/operators.jl

+# ensured that the eltype of `ret` is appropriate, and has zeroed the elements of `ret` (if
+# desired).
+
+function _A_mul_B!(ret::AbstractArray{T}, A, B) where {T<:JuMPTypes}


Nit: Spaces between binary operators, so T <: JuMPTypes.

Hmmm. No spaces surrounding <: seems to be the norm in the rest of the codebase. Seems best to change them all at once or not all to avoid inconsistent style - should I do that in this PR or leave until later?

We're fixing the style issues as we touch lines of code. It's pretty painful to update the whole code base at once, but you're welcome to if you'd like. Otherwise it's ok to have a mix of styles as long as we're moving in the right direction.

This seems like an easy one, I'll just go for it

EDIT: I overestimated my regex abilities and underestimated the formatting issues caused by naive find/replace here...so I only made this change to operators.jl.

jrevels · 2018-09-06T15:10:35Z

On the style nits - sorry, I didn't see http://www.juliaopt.org/JuMP.jl/latest/style.html until just now :) I'll get those fixes in soon.

codecov · 2018-09-06T15:55:50Z

Codecov Report

Merging #1456 into master will increase coverage by 0.5%.
The diff coverage is 87.7%.

@@            Coverage Diff            @@
##           master    #1456     +/-   ##
=========================================
+ Coverage   89.22%   89.72%   +0.5%     
=========================================
  Files          26       26             
  Lines        3723     3700     -23     
=========================================
- Hits         3322     3320      -2     
+ Misses        401      380     -21

Impacted Files	Coverage Δ
src/operators.jl	`93.66% <87.7%> (+5.35%)`	⬆️
src/JuMP.jl	`78.67% <0%> (+0.31%)`	⬆️
src/parseexpr.jl	`94.88% <0%> (+0.78%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4496a54...33e8359. Read the comment docs.

jrevels · 2018-09-10T15:16:21Z

Bump. This is ready (pending responses to my previous questions from the initial review).

…le guide

blegat · 2018-09-13T14:08:20Z

src/operators.jl


 Base.transpose(x::AbstractJuMPScalar) = x

 # Can remove the following code once == overloading is removed

-function Compat.LinearAlgebra.issymmetric(x::Matrix{T}) where T<:JuMPTypes
+function Compat.LinearAlgebra.issymmetric(x::Matrix{T}) where {T <: JuMPTypes}


Why use { and } when it is not a one line function ?

stylistic consistency (this is also more readable IMO, but that's subjective)

There probably should just be a rule in the style guide about the desired style here

blegat · 2018-09-13T14:14:30Z

src/operators.jl

-        _sizehint_expr!(q, n)
-        for k ∈ 1:n
-            tmp = convert(T, lhs[k,i]*rhs[k,j]) # transpose
+function fill_with_zeros!(A, ::Type{T}) where {T}


Isn't A an AbstractArray{T} ? Why is the second argument needed ?

It's bad practice to dispatch on element type in cases like these; the eltype of A and T only need be related by a subtype relation, not equality.

blegat · 2018-09-13T14:16:08Z

src/operators.jl

+# Don't do size checks here; defer that to `_A_mul_B(A, B)`.
+function _A_mul_B_ret(A, B, dims...)
+    T = _A_mul_B_eltype(eltype(A), eltype(B))
+    ret = Array{T}(undef, _A_mul_B_ret_dims(A, B))


Why not do zeros(T, ...) instead of Array... followed by fill_with_zeros! ?

I just carried this pattern over from the old code. It does leave a nice path to generalize to non-Array rets later, but that's probably premature.

I could replace it with zeros if we want to make that change.

zeros is broken for JuMP types because zero returns a mutable value.

Huh, TIL

julia> mutable struct Foo end julia> Base.zero(::Type{Foo}) = Foo() julia> x = zeros(Foo, 2) 2-element Array{Foo,1}: Foo() Foo() julia> x[1] === x[2] true

That's arguably a bug in Base's implementation. If I wanted a single instance, I would've just used fill(Foo(), 2)...

mlubin · 2018-09-15T01:22:41Z

src/operators.jl

+end
+
+###############################################################################
+# `_A_mul_B!(ret, A, B)` adds the result of `A*B` into the buffer `ret`. We roll our own


"code block comment separator" format for JuMP?

No, not really. I thought about it briefly but don't have a clear answer. Maybe more consistent use of empty lines would make it easier to separate sections without large horizontal comment bars.

mlubin · 2018-09-15T01:29:25Z

test/operator.jl

-            @test JuMP.isequal_canonical((2 .* x) ./ 3, Matrix((2 .* y) ./ 3))
-            @test JuMP.isequal_canonical(2 .* (x ./ 3), Matrix(2 * (y ./ 3)))
-            @test JuMP.isequal_canonical((x[1,1],) .* A, Matrix((x[1,1],) .* B))
+            z = JuMP.densify_with_jump_eltype((2 .* y) ./ 3)


Using an internal JuMP function to construct the expected answer seems unnecessary and makes it harder to verify that the test is correct.

Yeah - before this PR, these tests' reliance on internal constructors was obscured by the fact that the constructors were pirated overloads of Matrix. Now that it's easily visible where this is happening, I've added a TODO to refactor the tests

mlubin · 2018-09-15T01:29:31Z

test/operator.jl

-            @test JuMP.isequal_canonical((2 .* x) ./ 3, Matrix((2 .* y) ./ 3))
-            @test JuMP.isequal_canonical(2 .* (x ./ 3), Matrix(2 * (y ./ 3)))
-            @test JuMP.isequal_canonical((x[1,1],) .* A, Matrix((x[1,1],) .* B))
+            z = JuMP.densify_with_jump_eltype((2 .* y) ./ 3)


These tests are really poorly structured but I'll let this go for now. Unit tests should test only one thing and not refer to variables like y that are defined far away.

jrevels force-pushed the jr/opsupdate branch 3 times, most recently from 65769b4 to afd0e11 Compare September 5, 2018 20:37

jrevels added 2 commits September 5, 2018 16:53

clean up some linear algebra primitives for JuMP eltypes

a5ed161

remove inappropriate Matrix constructor usage in tests

7c67ff8

jrevels force-pushed the jr/opsupdate branch from afd0e11 to 7c67ff8 Compare September 5, 2018 21:01

fix sparse matmul method ambiguities

b5d3886

mlubin reviewed Sep 6, 2018

View reviewed changes

fix style nits

a19ac99

jrevels added 4 commits September 6, 2018 12:03

wrap scalar broadcast overload in VERSION check

bbbe2a6

pull densification call out from argument position in test

283dcf4

fix typo causing v0.6 failure

29299b4

add comment explaining some sparse arithmetic overloads

e456afc

fix <: and type parameter spacing for operators.jl according to sty…

33e8359

…le guide

blegat reviewed Sep 13, 2018

View reviewed changes

jrevels mentioned this pull request Sep 13, 2018

zeros etc. is arguably broken for mutable element types JuliaLang/julia#29168

Closed

mlubin reviewed Sep 15, 2018

View reviewed changes

jrevels and others added 2 commits September 15, 2018 11:51

add test refactor TODO

25e159f

Tweak TODO wording

ac7e887

mlubin merged commit 7ada455 into master Sep 15, 2018

jrevels deleted the jr/opsupdate branch September 15, 2018 15:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

clean up some linear algebra primitives for JuMP eltypes #1456

clean up some linear algebra primitives for JuMP eltypes #1456

jrevels commented Sep 5, 2018 •

edited

Loading

mlubin left a comment

mlubin Sep 6, 2018

jrevels Sep 6, 2018

mlubin Sep 6, 2018

mlubin Sep 6, 2018

mlubin Sep 6, 2018

mlubin Sep 6, 2018

jrevels Sep 6, 2018

mlubin Sep 15, 2018

mlubin Sep 6, 2018

jrevels Sep 6, 2018

mlubin Sep 6, 2018

mlubin Sep 6, 2018

jrevels Sep 6, 2018

mlubin Sep 6, 2018

jrevels Sep 6, 2018

mlubin Sep 10, 2018 •

edited

Loading

jrevels Sep 11, 2018 •

edited

Loading

jrevels commented Sep 6, 2018

codecov bot commented Sep 6, 2018 •

edited

Loading

jrevels commented Sep 10, 2018

blegat Sep 13, 2018

jrevels Sep 13, 2018

blegat Sep 13, 2018

jrevels Sep 13, 2018

blegat Sep 13, 2018

jrevels Sep 13, 2018

mlubin Sep 13, 2018

jrevels Sep 13, 2018

mlubin Sep 15, 2018

mlubin Sep 15, 2018

jrevels Sep 15, 2018

mlubin Sep 15, 2018


		densify_with_jump_eltype(x::AbstractMatrix) = convert(Matrix, x)

		function densify_with_jump_eltype(x::SparseMatrixCSC{V}) where V<:AbstractVariableRef


		Base.:*(x::AbstractArray{T}) where {T<:JuMPTypes} = x

		Base.:/(A::SparseMatrixCSC{T}, B::Number) where {T<:JuMPTypes} = SparseMatrixCSC(A.m, A.n, copy(A.colptr), copy(A.rowval), A.nzval ./ B)

		end

		Base.:(A::Number, B::SparseMatrixCSC{T}) where {T<:JuMPTypes} = SparseMatrixCSC(B.m, B.n, copy(B.colptr), copy(B.rowval), A . B.nzval)

clean up some linear algebra primitives for JuMP eltypes #1456

clean up some linear algebra primitives for JuMP eltypes #1456

Conversation

jrevels commented Sep 5, 2018 • edited Loading

mlubin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlubin Sep 10, 2018 • edited Loading

Choose a reason for hiding this comment

jrevels Sep 11, 2018 • edited Loading

Choose a reason for hiding this comment

jrevels commented Sep 6, 2018

codecov bot commented Sep 6, 2018 • edited Loading

Codecov Report

jrevels commented Sep 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrevels commented Sep 5, 2018 •

edited

Loading

mlubin Sep 10, 2018 •

edited

Loading

jrevels Sep 11, 2018 •

edited

Loading

codecov bot commented Sep 6, 2018 •

edited

Loading