adjoint/transpose transcend material concerns #25364

Sacha0 · 2018-01-03T05:02:58Z

This pull request is the next step in the JuliaLang/LinearAlgebra.jl#57 series after #24969, #25083, #25125, #25148, and #25217. Specifically, this pull request makes adjoint/transpose uniformly lazy and extends ~~collect~~ copy for materialization.

Remaining tasks, for this or a downstream pull request:

~~Transform most Adjoint/Transpose calls to adjoint/transpose.~~ done here
~~Lower ' directly to adjoint (thereby fixing method overwrites during build).~~ done here
Make Adjoint/Transpose behave like proper constructors.

Let's see what CI says. Thanks and best!

mbauman · 2018-01-03T21:49:40Z

base/linalg/bitarray.jl

@@ -123,7 +123,7 @@ end

 ## Structure query functions

-issymmetric(A::BitMatrix) = size(A, 1)==size(A, 2) && count(!iszero, A - transpose(A))==0
+issymmetric(A::BitMatrix) = size(A, 1)==size(A, 2) && count(!iszero, A - collect(A'))==0


Just curious: is your rationale for collecting here to ensure we still hit an optimized -(::BitMatrix, ::BitMatrix) method? If that's the case it may be worth keeping note of this so we can revisit this PR for more ::Adjoint method specializations in the future.

Partly, yes :). During the relevant transformation pass, preserving behavior was my primary concern. And for some array types, the rewrite above was strictly necessary to preserve behavior. (For example, S + Adjoint(S) for sparse matrix S hits an AbstractArray + fallback yielding an Array rather than the former S + adjoint(S)'s SparseMatrixCSC. Hence the transformation S + adjoint(S) -> S + collect(S') rather than S + S', though S + S' should work eventually.) And indeed, after the gross functionality is complete quite a bit of cleanup will be in order :). #25331 is my present breadcrumb for this particular issue.

mbauman · 2018-01-03T21:54:56Z

base/linalg/generic.jl

@@ -986,7 +986,7 @@ function ishermitian(A::AbstractMatrix)
        return false
    end
    for i = indsn, j = i:last(indsn)
-        if A[i,j] != adjoint(A[j,i])
+        if A[i,j] != Adjoint(A[j,i])


Why are you favoring the uppercase constructors in these cases?

The simple answer is that this pull request presently represents an intermediate state which will change substantially on next push. Specifically, most uppercase calls should then disappear :).

mbauman · 2018-01-03T21:57:52Z

base/number.jl

 first(x::Number) = x
 last(x::Number) = x
-copy(x::Number) = x  # some code treats numbers as collection-like
+copy(x::Number) = x
+collect(x::Number) = x


This may be worth calling out and thinking through a bit more. I expected collect(1) to return [1] since numbers are iterable "collections". In fact, right now collect(1) = fill(1) (that is, a zero-d array).

I get why this change is needed, but I'm not entirely sold that it's the right answer.

Agreed, I'm not entirely sold on it either :).

I'm also -1 on this definition.

Sacha0 · 2018-01-04T18:21:20Z

(Marking triage for collect discussion.)

vtjnash · 2018-01-04T20:44:25Z

From discussion on triage, there was some consensus to go with the following:

deprecate collect name:
- use Array constructor instead in some cases (usually for realizing lazy operations like iterators)
- use copy in other cases (probably most of these)
  - define copy(x::AbstractArray) = Array(x) (or some equivalent like copyto!(similar(Array, x), x))
  - encourage specializations of copy when there's a better definition which can fold away extra wrappers or, failing that, can return an object of the same type

Sacha0 · 2018-01-05T04:51:48Z

Rebased and completed rewrites to adjoint/transpose in base, test, and stdlib. Passing sparse, linalg, and stdlib test suites locally. So this pull request should be in shape after a sweep to replace collect methods/calls with copy as per triage. Best!

Sacha0 · 2018-01-05T20:58:52Z

Rebased, squashed out all intermediate commits in the adjoint/transpose lazification process, and added a commit transforming collect to copy as per triage. The last commit is #25418, which I will drop once #25418 merges. Passed all impacted test suites locally. Best!

Sacha0 · 2018-01-06T00:24:14Z

Added a commit lowering ' directly to adjoint and further cleaned up the commit history; the lowering change incidentally fixes the method overwrite warning during build mentioned in #25212. I plan to make the Adjoint/Transpose constructor changes in a separate pull request. So this pull request should now be in shape. Best!

mbauman

LGTM! Very nice work.

mbauman · 2018-01-06T00:55:55Z

base/pkg/resolve/maxsum.jl

@@ -122,7 +122,7 @@ mutable struct Graph
                        adjdict[p0][p1] = j1

                        bm = trues(spp[p1], spp[p0])
-                        bmt = adjoint(bm)
+                        bmt = trues(spp[p0], spp[p1])


Wow, your re-write here is way better. Bravo for not just zoning out (like I did when this change caught me off guard).

One small step towards atonement for the horde of ghastly transformations I introduced in #24969 😉.

Sacha0 · 2018-01-06T21:18:43Z

Rebased out the last commit with #25418 in. With Matt and CI approving, absent objections or requests for time I plan to merge these changes Sunday evening PT or later. Best! :)

…ons.

…vers.

Sacha0 · 2018-01-08T18:55:10Z

Rebased out conflict. Absent objections I plan to merge these changes once CI clears. Best!

…uliaLang#25364).

Sacha0 · 2018-01-09T01:30:51Z

(AV and FreeBSD failures are timeouts without apparent issue prior.)

Sacha0 · 2018-01-09T01:31:16Z

Thanks all! :)

…uliaLang#25364).

Sacha0 added the linear algebra Linear algebra label Jan 3, 2018

Sacha0 force-pushed the lowercase branch from e40eb13 to 29c2d79 Compare January 3, 2018 20:42

mbauman reviewed Jan 3, 2018

View reviewed changes

Sacha0 force-pushed the lowercase branch 3 times, most recently from ab71b79 to e2faff7 Compare January 4, 2018 05:36

Sacha0 added the triage This should be discussed on a triage call label Jan 4, 2018

Sacha0 mentioned this pull request Jan 4, 2018

remove 0.6 deprecations #25312

Merged

Sacha0 force-pushed the lowercase branch 2 times, most recently from f1b9de8 to 375d957 Compare January 4, 2018 19:09

Sacha0 removed the triage This should be discussed on a triage call label Jan 5, 2018

Sacha0 force-pushed the lowercase branch from 375d957 to c0affe7 Compare January 5, 2018 04:50

Sacha0 force-pushed the lowercase branch 2 times, most recently from a6ea332 to e842f27 Compare January 5, 2018 20:54

Sacha0 force-pushed the lowercase branch 2 times, most recently from 3663550 to 27aaaf3 Compare January 6, 2018 00:12

Sacha0 changed the title ~~[WIP] adjoint/transpose transcend material concerns~~ adjoint/transpose transcend material concerns Jan 6, 2018

mbauman approved these changes Jan 6, 2018

View reviewed changes

Sacha0 force-pushed the lowercase branch from 27aaaf3 to f8a7909 Compare January 6, 2018 21:17

This was referenced Jan 6, 2018

move SparseArrays to stdlib #25249

Merged

rename SparseMatrixCSC to SparseMatrix #25151

Closed

Sacha0 added 3 commits January 8, 2018 10:54

Make adjoint/transpose lazy with copy for materialization.

16a944e

Replace Adjoint/Transpose with adjoint/transpose throughout test/.

43b2cfb

Replace Adjoint/Transpose with adjoint/transpose throughout base/.

dd9cef8

Sacha0 added 4 commits January 8, 2018 10:54

Replace Adjoint/Transpose with adjoint/transpose throughout deprecati…

6a21539

…ons.

Replace Adjoint/Transpose with adjoint/transpose in IterativeEigensol…

1aef8e0

…vers.

Replace Adjoint/Transpose with adjoint/transpose in SuiteSparse.

70aed06

Lower ' directly to adjoint.

ac18317

Sacha0 force-pushed the lowercase branch from f8a7909 to ac18317 Compare January 8, 2018 18:54

Sacha0 added a commit to Sacha0/julia that referenced this pull request Jan 8, 2018

Replace Adjoint/Transpose with adjoint/transpose (instances missed in J…

e6829b2

…uliaLang#25364).

Sacha0 merged commit 09f7213 into JuliaLang:master Jan 9, 2018

Sacha0 deleted the lowercase branch January 9, 2018 01:31

Sacha0 added a commit to Sacha0/julia that referenced this pull request Jan 9, 2018

Replace Adjoint/Transpose with adjoint/transpose (instances missed in J…

29dc306

…uliaLang#25364).

This was referenced Jan 9, 2018

make Adjoint/Transpose behave like typical constructors #25461

Merged

revise .' deprecation message and add related TODOs to base/deprecated.jl #25463

Merged

Sacha0 added a commit to Sacha0/julia that referenced this pull request Jan 9, 2018

Replace Adjoint/Transpose with adjoint/transpose (instances missed in J…

ba21f20

…uliaLang#25364).

mbauman added the needs news A NEWS entry is required for this change label Jan 18, 2018

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

stevengj mentioned this pull request Aug 31, 2020

copy not implemented for String #31995

Closed

This was referenced Oct 23, 2020

Transposes and adjoints of triangular matrices JuliaLang/LinearAlgebra.jl#774

Closed

Swap adjtrans and triangular wrappers #38168

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adjoint/transpose transcend material concerns #25364

adjoint/transpose transcend material concerns #25364

Sacha0 commented Jan 3, 2018 •

edited

Loading

mbauman Jan 3, 2018

Sacha0 Jan 3, 2018

mbauman Jan 3, 2018

Sacha0 Jan 3, 2018

mbauman Jan 3, 2018

Sacha0 Jan 3, 2018

JeffBezanson Jan 4, 2018

Sacha0 commented Jan 4, 2018

vtjnash commented Jan 4, 2018

Sacha0 commented Jan 5, 2018

Sacha0 commented Jan 5, 2018

Sacha0 commented Jan 6, 2018

mbauman left a comment

mbauman Jan 6, 2018

Sacha0 Jan 6, 2018

Sacha0 commented Jan 6, 2018

Sacha0 commented Jan 8, 2018

Sacha0 commented Jan 9, 2018

Sacha0 commented Jan 9, 2018

adjoint/transpose transcend material concerns #25364

adjoint/transpose transcend material concerns #25364

Conversation

Sacha0 commented Jan 3, 2018 • 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

Sacha0 commented Jan 4, 2018

vtjnash commented Jan 4, 2018

Sacha0 commented Jan 5, 2018

Sacha0 commented Jan 5, 2018

Sacha0 commented Jan 6, 2018

mbauman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sacha0 commented Jan 6, 2018

Sacha0 commented Jan 8, 2018

Sacha0 commented Jan 9, 2018

Sacha0 commented Jan 9, 2018

Sacha0 commented Jan 3, 2018 •

edited

Loading