Conversation
lkdvos
force-pushed
the
ld-algmerge
branch
3 times, most recently
from
d8adb73 to
6a2bd16
Compare
lkdvos
force-pushed
the
ld-algmerge
branch
2 times, most recently
from
ded565e to
841ebac
Compare
Jutho
left a comment
Jutho
left a comment
There was a problem hiding this comment.
I think this looks good. Does this in any way give rise to conflicts with the whole algorithm selection mechanism via keywords and symbols?
|
Not really, I think mostly the |
|
So, I actually did manage to break the whole type stability chain in the LTS version. As a possible solution, I know just @Jutho, @kshyatt , @mtfishman I'd like to hear opinions on this, I think this will save me a lot of annoyance in the long run, but might also lead to accidents if used incorrectly, so there is definitely some trade-off there |
|
What about only calling |
|
Hmm, I think in that case it might be better to simply eat the type-instability in LTS, since then we still have the hazard of causing a hang somewhere but only on specific versions of Julia, which might lead to issues that are even harder to debug. It's just really unfortunate that there is some heuristic about how deep the compiler propagates constants, so even in the current code I assume there are ways to nest functions enough such that these function calls become type unstable... One other way I could approach this is to simply change the |
| return LQViaTransposedQR(qr_alg) | ||
| end | ||
| function MatrixAlgebraKit.default_svd_algorithm(::Type{Base.ReshapedArray{T, 2, SubArray{T, 1, A, Tuple{UnitRange{Int}}, true}, Tuple{}}}; kwargs...) where {T <: BlasFloat, A <: CuVecOrMat{T}} | ||
| const BlockView{T, A} = Base.ReshapedArray{T, 2, SubArray{T, 1, A, Tuple{UnitRange{Int}}, true}} |
There was a problem hiding this comment.
Is this specific for TensorKit?
Do we need the default_driver and default_algorithm primitives to operate on types, or could we have some
default_driver(A::ReshapedArray{T,2}) = default_driver(parent(A))
I guess that can even work in type domain
default_driver(::Type{<:ReshapedArray{T,N,A}}) where {T,N,A} = default_driver(A)
default_driver(::Type{<:SubArray{T,N,A}}) where {T,N,A} = default_driver(A)
?
There was a problem hiding this comment.
Is this specific for TensorKit?
Yes, we were unhappy with it at the time but couldn't really see a better way around it now. With the change of blocktype that's waiting to be merged it may be unnecessary?
There was a problem hiding this comment.
But you are right, I can make a generic implementation for default_householder_driver since that one is not being called in the type domain. Additionally, I like the type-domain default algorithm things as well, so I might also migrate that (less specializations in the extensions is easier to manage)!
There was a problem hiding this comment.
what change of blocktype? To return it as StridedView. Was this not causing all kinds of other issues? I don't think StridedView objects would currently be gracefully handled by MatrixAlgebraKit.
4 participants
This is a start for tackling #176, while refactoring some of the QR/LQ implementation to share more code between the different
backends-> drivers.As usual, to keep this somewhat contained I only looked at QR/LQ decompositions to allow for some discussion first.
The next steps would be to include the SVD and eigenvalue decompositions in a similar manner.
From what I can tell, I managed to implement this in a non-breaking way, deprecating the previous
*_Householder*structs without removing them.