sunset linalg jazz #25217

Sacha0 · 2017-12-21T00:41:50Z

This pull request is the next step towards #5332 after #24969, #25083, #25125, and #25148, completing item 18 and part of 19 in #24969's OP's task list. Specifically, this pull request: (1) deprecates all A[ct]_(mul|ldiv|rdiv)_B[ct][!] methods in favor of *, /, \, mul!, ldiv!, and rdiv!; (2) completes the transition from ConjRowVector/RowVector to Adjoint/Transpose by deprecating the former's constructors with pointers to the latter (while preserving associated functionality for the deprecation period) ; and (3) deprecates adjoint(v)/transpose(v) for v<:AbstractVector, which were already lazy, to Adjoint(v)/Transpose(v).

This pull request largely completes the work towards #5332. The remaining certain work is a solid news item, documentation, and a bit of cleanup; I plan to wrap those into future pull requests after sorting out other things feature-y. The remaining uncertain work / design question is what to do about adjoint/transpose for matrices; hopefully triage can sort that point out tomorrow and I will implement the decision in a downstream pull request.

Thanks and best!

StefanKarpinski · 2017-12-21T00:43:28Z

base/exports.jl

-    At_mul_Bt,
-    At_mul_Bt!,
-    At_rdiv_B,
-    At_rdiv_Bt,


Oh, man. That is satisfying.

Sacha0 · 2017-12-21T00:48:43Z

base/linalg/rowvector.jl

@@ -1,5 +1,7 @@
 # This file is a part of Julia. License is MIT: https://julialang.org/license

+# TODO: remove the type stubs below between 0.7 and 1.0


I left these type stubs in place to preserve these types' concatenation behavior during the deprecation period. (These types' concatenation behavior is defined through a set of large unions including these types in base/sparse/sparsevector.jl, which loads before base/deprecated.jl.)

Sacha0 · 2017-12-21T00:49:59Z

base/sparse/sparsevector.jl

-const _TypedDenseConcatGroup{T} = Union{Vector{T}, Adjoint{T,Vector{T}}, Transpose{T,Vector{T}}, RowVector{T,Vector{T}}, Matrix{T}, _Annotated_Typed_DenseArrays{T}}
+const _SparseConcatGroup = Union{Vector, Adjoint{<:Any,<:Vector}, Transpose{<:Any,<:Vector}, Base.LinAlg.RowVector{<:Any,<:Vector}, Matrix, _SparseConcatArrays, _Annotated_SparseConcatArrays, _Annotated_DenseArrays}
+const _DenseConcatGroup = Union{Vector, Adjoint{<:Any,<:Vector}, Transpose{<:Any,<:Vector}, Base.LinAlg.RowVector{<:Any, <:Vector}, Matrix, _Annotated_DenseArrays}
+const _TypedDenseConcatGroup{T} = Union{Vector{T}, Adjoint{T,Vector{T}}, Transpose{T,Vector{T}}, Base.LinAlg.RowVector{T,Vector{T}}, Matrix{T}, _Annotated_Typed_DenseArrays{T}}


RowVector remains in these unions to preserve its concatenation behavior during the deprecation period.

Sacha0 · 2017-12-21T00:50:55Z

test/linalg/rowvector.jl

-
-    # setindex!(v::RowVector, ...) should return v rather than v's parent
-    @test setindex!(RowVector([1, 2, 3]), 4, 1)::RowVector == [4 2 3]
-end


Equivalents of almost all of these tests (and more besides) now exercise Adjoint/Transpose.

Sacha0 · 2017-12-21T00:51:45Z

base/deprecated.jl

-@deprecate RowVector{T}(n::Int) where {T}               RowVector{T}(uninitialized, n)
-@deprecate RowVector{T}(n1::Int, n2::Int) where {T}     RowVector{T}(uninitialized, n1, n2)
-@deprecate RowVector{T}(n::Tuple{Int}) where {T}        RowVector{T}(uninitialized, n)
-@deprecate RowVector{T}(n::Tuple{Int,Int}) where {T}    RowVector{T}(uninitialized, n)


Moved below among other things RowVector.

Sacha0 · 2017-12-21T01:04:11Z

I would like to give tremendous thanks to (among several other parties!) @andreasnoack, @simonbyrne, @mbauman, and @andyferris, each of whom at one point or another built a branch exploring these changes, and also for Andreas's, Simon's, and Andy's greatly helpful direct involvement in preparing this series of pull requests. ❤️!

andreasnoack · 2017-12-21T09:39:29Z

This is really great and an incredible amount of work. Thank you so much for doing it.

stevengj · 2017-12-21T17:45:07Z

If we have adjoint(v)/transpose(v) for matrices, shouldn't we leave them for vectors in order to facilitate writing generic code?

StefanKarpinski · 2017-12-21T19:36:29Z

Triage wants to call the function to materialize a final, non-lazy result collect consistent with its current meaning to collect all the values in iterators into a concrete vector.

Jutho · 2017-12-21T23:49:03Z

Thanks for all of this. Wasn't the conclusion from #16772 that 2-arg mul! methods would get a different name that indicates which argument is overwritten?

Sacha0 · 2017-12-22T00:06:13Z

Wasn't the conclusion from #16772 that 2-arg mul! methods would get a different name that indicates which argument is overwritten?

Separate work :).

andyferris · 2017-12-22T11:39:28Z

To clarify - is adjoint(::AbstractVector) only deprecated because it's meaning will change to be create an Adjoint instead of a RowVector? And we'll reintroduce a method for this in v1.0?

Sacha0 · 2017-12-23T03:04:04Z

If we have adjoint(v)/transpose(v) for matrices, shouldn't we leave them for vectors in order to facilitate writing generic code?

To clarify - is adjoint(::AbstractVector) only deprecated because it's meaning will change to be create an Adjoint instead of a RowVector? And we'll reintroduce a method for this in v1.0?

In its present form, adjoint is like a box of chocolates: you never know what you're gonna get. Sometimes it's eager. Sometimes it's lazy. When it's lazy, sometimes it yields Adjoint (ConjRowVector), others it yields Transpose (RowVector). A somewhat confusing situation and tricky to write against generically :).

So recent discussion has favored having distinct operations that are clearly / uniformly lazy on the one hand and clearly / uniformly eager on the other. Hence the deprecation of adjoint(v)/transpose(v) for vectors in this pull request, with the fate of adjoint(A)/transpose(A) for matrices TBD as of the OP, and more recent leanings towards having only the lazy ops + explicit collection. Best!

andyferris · 2017-12-23T12:39:16Z

Hmm... I have to say I'm still a bit confused what the plan is here. I guess I might have missed something, sorry! Let me nut this out:

adjoint is a perfectly valid mathematical function (like conj) and IMO clearly should stay (lazy or eager). We need that adjoint(adjoint(v)) is a no-op (functionally speaking - it might e.g. return a copy of v) and that Adjoint(Adjoint(v)) constructs an Adjoint (it can't return an AbstractVector). Also, the outermost Adjoint(...) should ~~definitely~~ possibly be the (very shallow?) copy-constructor Adjoint(::Adjoint), if Adjoint is a type. So Adjoint(Adjoint(v)) == Adjoint(v) because that's just how types and constructors work in Julia, right? Otherwise I'd find that confusing. So, as I see it, we can't have Adjoint being the lazy but otherwise functionally equivalent replacement of adjoint.

I definitely agree that it's messy when some methods of a function are eager and some are lazy - but I thought the plan here had been to make all transpose and adjoint calls lazy (and document that)? I would advocate for that, anyway (it's really easy to use copy, after all). I feel that the most breaking thing here (in this series of PRs) will be that transpose and adjoint of matrices will no longer be copying - the renaming of RowVector seems pretty cosmetic.

Sacha0 · 2017-12-23T21:17:30Z

Much thanks for the thoughtful and cogent post Andy! I lack strong sentiments regarding which name should have Adjoint's present semantics. (Minimizing breakage was the primary reason for Adjoint having its present semantics, but at this point what's another grain of sand on the beach 😉.) If general sentiment favors giving Adjoint's present semantics to adjoint and accepting the additional breakage, I would be delighted to drop this pull request's last commit / downstream work in progress and pivot to the plan just above. Thoughts all? In the interest of seeing this and downstream work through in a timely manner, rapid formation of a quorum either way would be lovely.

jekbradbury · 2017-12-24T19:24:36Z

I'm developing a new array-like wrapper type over at https://github.com/jekbradbury/Minibatch.jl, and I'm coming at this as a relative beginner. From that perspective I think there are some things that might be worth mentioning:

It would be nice if there were a single, obvious level of abstraction where I should add multiplication methods (i.e., either to Base.:* or to LinAlg.mul(!)). Looks like there is with this proposal (the latter) but not currently (I could add methods for all the jazzy functions but it's easier to let them use fallbacks and just add methods to *).
I'd like to be able to indicate that I want the lazy adjoint wrapper to "pass through" my type. So if a isa MyType{Matrix{Float64}} then a' should have type MyType{Adjoint{Matrix{Float64}}} rather than Adjoint{MyType{Matrix{Float64}}}, since the Adjoint wrapper shouldn't have to know how to deal with my type, while my type knows exactly how to deal with an adjoint wrapper (pass it on to the mul functions we're calling). This suggests that lowering a' directly to Adjoint(a) is not ideal, while lowering it to adjoint(a) (with a fallback method adjoint(a::AbstractMatrix) = Adjoint(a)) would work very cleanly.

Sacha0 · 2017-12-25T02:51:32Z

Rebased and dropped the last commit, leaving only uncontroversial changes in this pull request. Absent objections or requests for time, I plan to merge these changes tomorrow evening PT at the earliest.

With the support above for transferring Adjoint/Transpose's semantics to adjoint/transpose, and additional support voiced on slack, I nixed existing work in progress and instead am pursuing the preceding semantics transfer alongside introduction of collect for materialization. Thanks all! :)

…iv!, and rdiv!.

…tion.

Sacha0 · 2017-12-28T18:57:45Z

(Marked triage for merge check.)

ararslan · 2017-12-28T20:31:43Z

IT'S HAPPENING!!!! 🎉

Sacha0 added deprecation This change introduces or involves a deprecation linear algebra Linear algebra needs news A NEWS entry is required for this change labels Dec 21, 2017

StefanKarpinski reviewed Dec 21, 2017

View reviewed changes

Sacha0 mentioned this pull request Dec 21, 2017

lazier, less-jazzy linalg internals #24969

Merged

Sacha0 commented Dec 21, 2017

View reviewed changes

jrevels added the triage This should be discussed on a triage call label Dec 21, 2017

StefanKarpinski removed the triage This should be discussed on a triage call label Dec 21, 2017

nalimilan mentioned this pull request Dec 21, 2017

Merge collect() into Vector()? #16029

Open

fredrikekre mentioned this pull request Dec 21, 2017

fix vec and diag for RowVector #25237

Closed

Sacha0 force-pushed the sunsetjazz branch from 9809ce6 to ce68669 Compare December 25, 2017 02:46

Sacha0 force-pushed the sunsetjazz branch 2 times, most recently from ebf71e7 to a3044d4 Compare December 25, 2017 22:50

Sacha0 added 2 commits December 25, 2017 18:24

Deprecate A[ct]_(mul|ldiv|rdiv)_B[ct][!] in favor f *, /, \, mul!, ld…

d83dca9

…iv!, and rdiv!.

Deprecate ConjArray as part of the ConjRowVector/RowVector implementa…

7c8068f

…tion.

Deprecate ConjRowVector/RowVector in favor of Adjoint/Transpose.

3ed7386

Sacha0 force-pushed the sunsetjazz branch from a3044d4 to 3ed7386 Compare December 26, 2017 02:25

Sacha0 referenced this pull request Dec 26, 2017

Disambiguate mul!, ldiv!, and rdiv! methods.

6fb85f0

This was referenced Dec 26, 2017

remove legacy at-pure annotation from linalg/adjtrans #25272

Merged

RFC: introduce runtime representation of broadcast fusion #23692

Closed

Sacha0 added the triage This should be discussed on a triage call label Dec 28, 2017

JeffBezanson removed the triage This should be discussed on a triage call label Dec 28, 2017

Sacha0 merged commit eb91796 into JuliaLang:master Dec 28, 2017

Sacha0 deleted the sunsetjazz branch December 28, 2017 20:32

This was referenced Jan 3, 2018

adjoint/transpose transcend material concerns #25364

Merged

make Adjoint/Transpose behave like typical constructors #25461

Merged

mbauman mentioned this pull request Jan 12, 2018

Arraypocalypse Now and Then #13157

Closed

27 tasks

andreasnoack mentioned this pull request Mar 3, 2018

Define factorize(Adjoint) to make e.g. inv(Adjoint) work (in most cases) #26302

Merged

stevengj mentioned this pull request May 5, 2018

RowVector issue #24590 #26992

Closed

KristofferC removed the needs news A NEWS entry is required for this change label Nov 13, 2018

hochB mentioned this pull request Dec 30, 2018

A[t]_mul_B[!] deprecated upstream jump-dev/JuMP.jl#1716

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sunset linalg jazz #25217

sunset linalg jazz #25217

Sacha0 commented Dec 21, 2017

StefanKarpinski Dec 21, 2017

Sacha0 Dec 21, 2017

Sacha0 Dec 21, 2017

Sacha0 Dec 21, 2017

Sacha0 Dec 21, 2017

Sacha0 commented Dec 21, 2017

andreasnoack commented Dec 21, 2017

stevengj commented Dec 21, 2017

StefanKarpinski commented Dec 21, 2017

Jutho commented Dec 21, 2017

Sacha0 commented Dec 22, 2017

andyferris commented Dec 22, 2017

Sacha0 commented Dec 23, 2017

andyferris commented Dec 23, 2017 •

edited

Loading

Sacha0 commented Dec 23, 2017

jekbradbury commented Dec 24, 2017 •

edited

Loading

Sacha0 commented Dec 25, 2017

Sacha0 commented Dec 28, 2017

ararslan commented Dec 28, 2017

		@@ -1,5 +1,7 @@
		# This file is a part of Julia. License is MIT: https://julialang.org/license

		# TODO: remove the type stubs below between 0.7 and 1.0

sunset linalg jazz #25217

sunset linalg jazz #25217

Conversation

Sacha0 commented Dec 21, 2017

StefanKarpinski Dec 21, 2017

Choose a reason for hiding this comment

Sacha0 Dec 21, 2017

Choose a reason for hiding this comment

Sacha0 Dec 21, 2017

Choose a reason for hiding this comment

Sacha0 Dec 21, 2017

Choose a reason for hiding this comment

Sacha0 Dec 21, 2017

Choose a reason for hiding this comment

Sacha0 commented Dec 21, 2017

andreasnoack commented Dec 21, 2017

stevengj commented Dec 21, 2017

StefanKarpinski commented Dec 21, 2017

Jutho commented Dec 21, 2017

Sacha0 commented Dec 22, 2017

andyferris commented Dec 22, 2017

Sacha0 commented Dec 23, 2017

andyferris commented Dec 23, 2017 • edited Loading

Sacha0 commented Dec 23, 2017

jekbradbury commented Dec 24, 2017 • edited Loading

Sacha0 commented Dec 25, 2017

Sacha0 commented Dec 28, 2017

ararslan commented Dec 28, 2017

andyferris commented Dec 23, 2017 •

edited

Loading

jekbradbury commented Dec 24, 2017 •

edited

Loading