Repository navigation
Start on batched SVD support - #567
Conversation
lkdvos
left a comment
lkdvos
left a comment
There was a problem hiding this comment.
I haven't looked in too much detail at the actual implementation, but I think there's two more high-level comments/questions I have here:
I'm a bit surprised about actually trying to implement batched_f(::AbstractTensorMap), since that doesn't really feel like what is going on (it's a single tensor after all), and more importantly that is not super convenient to switch to from higher-level packages, since that doesn't involve switching algorithm and instead requires switching the function. I was somehow expecting that we would call f(::AbstractTensorMap), which would then automatically dispatch to batched_f on the blocks.
A second comment is that if I'm not mistaken there now is logic for deciding whether or not to batch in both MAK and also here, which is probably something to discuss: I would say we should either implement batched_f to handle how to decide grouping/padding/etc, or alternatively decide that here and pass on the 3D arrays, but in this latter case it seems like the batching logic shouldn't really be in MAK, (I'm also leaning slightly towards having that logic in MAK tbh).
|
Yeah I kind of did some necromancy on this after the MAK changes so I think this is helpful :). I was kind of torn about whether to "hide the ball" and just call |
|
Managed to offload nearly all the packing and decision making to MAK, the main thing is we need the PR over there to avoid trying to call something that doesn't (yet) exist for the LAPACK bindings. |
167ce1d to
6125746
Compare
3618dce to
e3f7cb7
Compare
Codecov Report✅ All modified and coverable lines are covered by tests.
... and 15 files with indirect coverage changes 🚀 New features to boost your workflow:
|
| U, S, Vᴴ = F | ||
| driver = get(alg.kwargs, :driver, MAK.DefaultDriver()) | ||
| cs = collect(union(blocksectors.((t, U, S, Vᴴ))...)) | ||
| if !isempty(cs) && MAK.supports_ragged_batch(MAK.$f!, alg, driver, storagetype(t)) |
There was a problem hiding this comment.
I just now noticed that we are passing both alg and driver here, is there a reason for this? Isn't driver already in alg? (I should have noticed this in the MAK reviews, apologies for bringing this up here)
There was a problem hiding this comment.
Nah no worries, I think we did this to be consistent between all the supports_*_batch methods
|
The |
ff5fd2d to
7f5c127
Compare
7f5c127 to
2d61b0b
Compare
| return F | ||
| end | ||
| foreachblock(t, U, S, Vᴴ) do _, (tblock, Fblocks...) | ||
| @timeit_debug GLOBAL_TIMER "dense: MatrixAlgebraKit" begin |
There was a problem hiding this comment.
If I remember correctly, in the mul! code etc, @lkdvos placed these @timeit insertions outside the for loop, to only have one entry for all the blocks together, and to reduce the overhead.
There was a problem hiding this comment.
Indeed that's the case for mul!, but for other factorizations (qr_compact! etc) the @timeit is inside the loop, so I chose consistency with those.
| $(f! in (:eig_vals!, :eigh_vals!, :project_hermitian!, :project_antihermitian!, :exponential!) && :(LinearAlgebra.checksquare(t))) | ||
| @timeit_debug GLOBAL_TIMER $(string(f!)) begin | ||
| foreachblock(t, N) do _, (tblock, Nblock) | ||
| @timeit_debug GLOBAL_TIMER "dense: MatrixAlgebraKit" begin |
There was a problem hiding this comment.
It seems here the @timeit statement is also inside the loop, so we should probably compare whether we are consistent between the decompositions and the mul! and other similar methods (if any)?
| return S | ||
| end | ||
| foreachblock(t, S) do _, (tblock, Sblock) | ||
| @timeit_debug GLOBAL_TIMER "dense: MatrixAlgebraKit" begin |
Jutho
left a comment
Jutho
left a comment
There was a problem hiding this comment.
Other than the @timeit_debug consistency comments, no further remarks from my side. This consistency comment can maybe be addressed in a follow-up PR (or simply by opening an issue for it for now).
No description provided.