LinAlg.fillslots! -> LinAlg.fillstored! #25030

fredrikekre · 2017-12-11T21:06:29Z

Sacha0 · 2017-12-12T03:26:11Z

base/linalg/special.jl

+# TODO: Add Diagonal to this method when 0.7 deprecations are removed
+function fill!(A::Union{Bidiagonal,Tridiagonal,SymTridiagonal}, x)
+    xT = convert(eltype(A), x)
+    (xT == zero(eltype(A)) || _small_enough(A)) && return fillstored!(A, xT)


iszero(xT)? :)

Sacha0 · 2017-12-12T03:27:38Z

base/linalg/special.jl

+function fill!(A::Union{Bidiagonal,Tridiagonal,SymTridiagonal}, x)
+    xT = convert(eltype(A), x)
+    (xT == zero(eltype(A)) || _small_enough(A)) && return fillstored!(A, xT)
+    throw(ArgumentError("array A of type $(typeof(A)) and size $(size(A)) can


Perhaps nix the A? The user will not be aware of the array's name in the method signature :).

Sacha0 · 2017-12-12T03:28:44Z

test/linalg/bidiag.jl

@@ -309,18 +309,18 @@ end
    @test promote(C,A) isa Tuple{Tridiagonal, Tridiagonal}
 end

-import Base.LinAlg: fillslots!, UnitLowerTriangular
-@testset "fill! and fillslots!" begin
+import Base.LinAlg: fillstored!, UnitLowerTriangular


Perhaps import -> using while touching this code?

Sacha0 · 2017-12-12T03:30:53Z

test/linalg/bidiag.jl

-import Base.LinAlg: fillslots!, UnitLowerTriangular
-@testset "fill! and fillslots!" begin
+import Base.LinAlg: fillstored!, UnitLowerTriangular
+@testset "fill! and fillstored!" begin
    let #fill!


This enclosing let seems vestigial; perhaps remove and de-indent while touching this code?

Sacha0 · 2017-12-12T03:32:57Z

base/linalg/triangular.jl

@@ -418,6 +418,8 @@ end
 scale!(A::Union{UpperTriangular,LowerTriangular}, c::Number) = scale!(A,A,c)
 scale!(c::Number, A::Union{UpperTriangular,LowerTriangular}) = scale!(A,c)

+fillstored!(A::AbstractTriangular, x) = (fill!(A.data, x); A)


I wonder whether we should constrain the mutated part of A.data in case something else aliases A.data?

It would be very weird to expect the following:

julia> A = fill(2., 2, 2); julia> T = UpperTriangular(A); julia> LinAlg.fillstored!(T, 0); julia> A 2×2 Array{Float64,2}: 0.0 0.0 2.0 0.0

I think those wrappers should have the license to do whatever they like with the ignored part. (Also, the ignored part is still stored so the function name implies that we will fill that too )

The behavior you highlight does not strike me as weird, but rather as desirable :). Consider, for example, that packing the factors from a decomposition into a single matrix is common. Were you using <:AbstractTriangular wrappers to work with / manipulate those wrappers externally (which we do in LinAlg IIRC), the behavior above is correct, whereas the implemented behavior silently corrupts other objects. Another angle from which to consider this point: Should fillstored!(view(A, 1:2, 1:2), 0) corrupt all other data in A? :)

A view clearly does not own it's wrapped Array while I would argue a type like UpperTriangular does.
We have no way in Julia to express these semantics but giving an Array to something like UpperTriangular and keep reusing that Array while having some expectations on how this type will modify it seems very suspicious and error prone. Ideally, there would be a way where the original data buffers could be "moved" to another binding while making it illegal to use the prevous one (std::movestyle).

However, since we don't have that, it is imo fair game for the type to do whatever you want with the passed in Array and no consideration has to be taken to any future use of this Array.

A view clearly does not own it's wrapped Array

<:AbstractTriangular are also views, and just as a SubArray does not own its parent's storage beyond the SubArray's purview, a <:AbstractTriangular should not either. Neither SubArrays nor <:AbstractTriangular allow mutation of the parent beyond the view's purview via setindex! and, insofar as I am aware, no other operations over such views allow/cause such mutation either. Implementations of operations over such views that do perform such mutation break the abstraction, and in this case unnecessarily so. Moreover, as mentioned above there are good reasons not to perform such mutation: Packed storage is common in numerical linear algebra, as in e.g. most dense triangular or orthogonal decompositions. See, e.g., base/linalg/lu.jl for operations over triangular views of subsections of packed decompositions that such mutation could jeopardize. Best!

Sacha0

Generally looks great! Thanks @fredrikekre! :)

(The only non-negligible review comment concerns the fillstored!(A::AbstractTriangular, x) definition. Not certain what's best.)

move implementations to more suitable places

Sacha0 · 2017-12-14T05:14:27Z

base/linalg/special.jl

+    xT = convert(eltype(A), x)
+    (iszero(xT) || _small_enough(A)) && return fillstored!(A, xT)
+    throw(ArgumentError("array of type $(typeof(A)) and size $(size(A)) can
+    not be filled with x=$x, since some of its entries are constrained."))


Perhaps nix the x=, given that what x refers to is not user-visible?

Sacha0 · 2017-12-14T05:19:03Z

test/linalg/bidiag.jl

+@testset "fill! and fillstored!" begin
+    let # fillstored!
+        A = Tridiagonal(randn(2), randn(3), randn(2))
+        @test fillstored!(A, 3) == Tridiagonal([3, 3.], [3, 3, 3.], [3, 3.])


No need for the trailing .s given that == does not check type equality?

Sacha0 · 2017-12-14T05:20:49Z

test/linalg/bidiag.jl

+        ]
+        for A in exotic_arrays
+            fill!(A, 0)
+            for a in A


Perhaps @test iszero(fill!(A, 0))?

Sacha0

Looks great! :) I will give this another pass tomorrow (with benefit of sleep).

Sacha0 · 2017-12-14T23:34:41Z

test/linalg/bidiag.jl

+        b = Bidiagonal(randn(1,1), :U)
+        st = SymTridiagonal(randn(1,1))
+        for x in (b, st)
+            @test Array(fill!(x, val)) == fill!(Array(x), val)
        end


Perhaps simplify to the following?

@test fill!(Bidiagonal(fill(0, 1, 1), :U), 2) == fill(2, 1, 1) @test fill!(SymTridiagonal(fill(0, 1, 1)), 2) == fill(2, 1, 1)

Sacha0 · 2017-12-14T23:35:30Z

test/linalg/bidiag.jl

+        S = SymTridiagonal(randn(3), randn(2))
+        @test fillstored!(S, 1) == SymTridiagonal([1,1,1], [1,1])
+        Ult = UnitLowerTriangular(randn(3,3))
+        @test fillstored!(Ult, 3) == UnitLowerTriangular([1 0 0; 3 1 0; 3 3 1])


Perhaps the other triangular types are worth testing as well?

Sacha0 · 2017-12-14T23:36:42Z

test/linalg/bidiag.jl

+        t = Tridiagonal(randn(3,3))
+        for x in (b, t, st)
+            @test_throws ArgumentError fill!(x, val)
+            @test Array(fill!(x, 0)) == fill!(Array(x), 0)


Perhaps simplify to @test fill!(x, 0) == ...?

Sacha0

Looks great! Thanks @fredrikekre! Mergeworthy insofar as I am concerned :).

fredrikekre added deprecation This change introduces or involves a deprecation linear algebra Linear algebra labels Dec 11, 2017

fredrikekre requested a review from Sacha0 December 11, 2017 21:06

Sacha0 reviewed Dec 12, 2017

View reviewed changes

Sacha0 approved these changes Dec 12, 2017

View reviewed changes

fredrikekre force-pushed the fe/fillstored branch from db22adb to bdae975 Compare December 12, 2017 07:30

fredrikekre mentioned this pull request Dec 12, 2017

deprecate spones #25037

Merged

fredrikekre added 2 commits December 12, 2017 20:07

rename LinAlg.fillslots! LinAlg.fillstored!

bc28fbf

rewrite fillstored!(::SpecialMatrix, x) without generated function

197ada2

move implementations to more suitable places

fredrikekre force-pushed the fe/fillstored branch 2 times, most recently from 68865e2 to 60f23df Compare December 13, 2017 17:17

implement fillband! and call it from fillstored!(::AbstractTriangular)

893281d

fredrikekre force-pushed the fe/fillstored branch from 60f23df to 893281d Compare December 13, 2017 21:15

fredrikekre requested a review from Sacha0 December 13, 2017 21:22

Sacha0 reviewed Dec 14, 2017

View reviewed changes

review fixes

69369a7

Sacha0 reviewed Dec 14, 2017

View reviewed changes

Sacha0 approved these changes Dec 14, 2017

View reviewed changes

fredrikekre merged commit 1f33cdd into JuliaLang:master Dec 14, 2017

fredrikekre deleted the fe/fillstored branch December 14, 2017 23:41

fredrikekre mentioned this pull request Dec 18, 2017

fill[stored]! for Symmetric and Hermitian matrices #25152

Merged

cormullion mentioned this pull request Mar 18, 2018

NEWS.md is getting a bit untidy #26508

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LinAlg.fillslots! -> LinAlg.fillstored! #25030

LinAlg.fillslots! -> LinAlg.fillstored! #25030

fredrikekre commented Dec 11, 2017 •

edited

Loading

Sacha0 Dec 12, 2017

Sacha0 Dec 12, 2017

Sacha0 Dec 12, 2017

Sacha0 Dec 12, 2017

Sacha0 Dec 12, 2017

fredrikekre Dec 12, 2017

Sacha0 Dec 12, 2017

KristofferC Dec 12, 2017 •

edited

Loading

Sacha0 Dec 12, 2017

Sacha0 left a comment

Sacha0 Dec 14, 2017 •

edited

Loading

Sacha0 Dec 14, 2017

Sacha0 Dec 14, 2017 •

edited

Loading

Sacha0 left a comment

Sacha0 Dec 14, 2017

Sacha0 Dec 14, 2017

Sacha0 Dec 14, 2017

Sacha0 left a comment

LinAlg.fillslots! -> LinAlg.fillstored! #25030

LinAlg.fillslots! -> LinAlg.fillstored! #25030

Conversation

fredrikekre commented Dec 11, 2017 • edited Loading

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

KristofferC Dec 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sacha0 left a comment

Choose a reason for hiding this comment

Sacha0 Dec 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sacha0 Dec 14, 2017 • edited Loading

Choose a reason for hiding this comment

Sacha0 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

Sacha0 left a comment

Choose a reason for hiding this comment

fredrikekre commented Dec 11, 2017 •

edited

Loading

KristofferC Dec 12, 2017 •

edited

Loading

Sacha0 Dec 14, 2017 •

edited

Loading

Sacha0 Dec 14, 2017 •

edited

Loading