fixed regression in sparse of adjoint of sparse (#28948) #28954
Conversation
ExpandingMan
changed the title
|
Ok, what's the deal with these "xcode8" tests, do we expect these to pass? Looks like there are lots of failures that seem to have absolutely nothing to do with this PR, though looking around I see other PR's passing it. |
You mean the Mac OSX tests? Yes we expect those to pass. There was one network failure so I restarted CI. |
|
For give my ignorance as to how the process works, but as this is essentially just a fix of a performance regression, I'm wondering if this can get a backport tag? On second thought I just realized that the tag would probably be added after approval 🤦♂️ . Nvm, I'm sure you guys will give the approprate tag. |
| end | ||
| function SparseMatrixCSC{Tv,Ti}(M::Transpose{Tv,SparseMatrixCSC{Tv,Ti}}) where {Tv,Ti} | ||
| copy(M) | ||
| end | ||
| SparseMatrixCSC(M::Adjoint{<:Any,<:SparseMatrixCSC}) = copy(M) | ||
| SparseMatrixCSC(M::Transpose{<:Any,<:SparseMatrixCSC}) = copy(M) |
There was a problem hiding this comment.
Thanks @ExpandingMan! :)
Perhaps completing the method set would be worthwhile, e.g. the sketch
SparseMatrixCSC(M::Adjoint{<:Any,<:SparseMatrixCSC}) = copy(M)
SparseMatrixCSC(M::Transpose{<:Any,<:SparseMatrixCSC}) = copy(M)
SparseMatrixCSC{Tv}(M::Adjoint{Tv,<:SparseMatrixCSC{Tv}}) where {Tv} = copy(M)
SparseMatrixCSC{Tv}(M::Transpose{Tv,<:SparseMatrixCSC{Tv}}) where {Tv} = copy(M)
SparseMatrixCSC{Tv,Ti}(M::Adjoint{Tv,SparseMatrixCSC{Tv,Ti}}) where {Tv,Ti} = copy(M)
SparseMatrixCSC{Tv,Ti}(M::Transpose{Tv,SparseMatrixCSC{Tv,Ti}}) where {Tv,Ti} = copy(M)
Regarding
Note that there is probably room for improvement here: performance of SparseMatrixCSC{T,Ti}(M::Adjoint{U,SparseMatrixCSC{U,Ti}}) where T \ne U will still be terrible, it's just not immediately obvious to me what the best solution to that is.
in combination with the preceding, the following sketch provides simple if suboptimal, but decent, implementations
SparseMatrixCSC{Tv}(M::Adjoint{<:Any,<:SparseMatrixCSC}) where {Tv} = SparseMatrixCSC{Tv}(copy(M))
SparseMatrixCSC{Tv}(M::Transpose{<:Any,<:SparseMatrixCSC}) where {Tv} = SparseMatrixCSC{Tv}(copy(M))
SparseMatrixCSC{Tv,Ti}(M::Adjoint{<:Any,<:SparseMatrixCSC}) where {Tv,Ti} = SparseMatrixCSC{Tv,Ti}(copy(M))
SparseMatrixCSC{Tv,Ti}(M::Transpose{<:Any,<:SparseMatrixCSC}) where {Tv,Ti} = SparseMatrixCSC{Tv,Ti}(copy(M))
The net number of method definitions could probably be reduced via LinearAlgebra.AdjOrTrans, e.g.
SparseMatrixCSC(M::Adjoint{<:Any,<:SparseMatrixCSC}) = copy(M)
SparseMatrixCSC(M::Transpose{<:Any,<:SparseMatrixCSC}) = copy(M)
collapses to
SparseMatrixCSC(M::AdjOrTrans{<:Any,<:SparseMatrixCSC}) = copy(M)
Best!
|
Ok, added the recommended methods. I did not use |
| end | ||
| function SparseMatrixCSC{Tv,Ti}(M::Transpose{Tv,SparseMatrixCSC{Tv,Ti}}) where {Tv,Ti} | ||
| copy(M) | ||
| end |
There was a problem hiding this comment.
Why use long-form definitions for these methods?
| end | ||
| function SparseMatrixCSC{Tv,Ti}(M::Transpose{<:Any,SparseMatrixCSC}) where {Tv,Ti} | ||
| SparseMatrixCSC{Tv,Ti}(copy(M)) | ||
| end |
There was a problem hiding this comment.
Likewise these long-form definitions? :)
|
I had left the other methods as blocks because they definitely go past my screen-split in vim 😉 . I checked the file though, and there are already other lines which are longer, so I collapsed them as well. |
There was a problem hiding this comment.
Looks great! Thanks @ExpandingMan! :)
Are these constructors already tested, or might they benefit from a little exercise?
|
I have tested these, but not at all thoroughly. If you want to do a quick sanity check it may be worthwhile. The "really scary" regression definitely seems to be fixed. (I think freebsd might need to be re-triggered.) |
KristofferC
added
the
potential benchmark
|
It feels a bit "silly" that we need all these definitions but if that is what it takes let's just do it. Will merge when FreeBSD finishes CI. Butting a benchmark label so if someone feels inclined can put this case into BaseBenchmarks.jl |
|
One could probably do a thorough pass through Is there some action required to re-trigger freebsd? |
|
Hm, I tried to do it manually but it didn't seem to take. Anyway, sparse tests succeeded on FreeBSD anyway so merging. |
This is a fix to #28948. What was happening is that
sparsewas causing a call to be made toSparseMatrixCSCwith explicit type parameters which was a fallback to an inefficient method, as without the methods I added onlySparseMatrixCSCwithout explicit type parameters knew how to do this operation efficiently.Note that there is probably room for improvement here: performance of
SparseMatrixCSC{T,Ti}(M::Adjoint{U,SparseMatrixCSC{U,Ti}})whereT \ne Uwill still be terrible, it's just not immediately obvious to me what the best solution to that is.