Use Testsuite for AD tests #126
Conversation
| L = kron(diagm(I_n), A) + kron(adjoint(B), diagm(I_m)) | ||
| x_vec = L \ -vec(C) | ||
| X = CuMatrix(reshape(x_vec, m, n))=# | ||
| hX = sylvester(collect(A), collect(B), collect(C)) |
There was a problem hiding this comment.
This is very awful but I wasn't able to find a correct way to do it in five minutes so there you go
There was a problem hiding this comment.
Is there any chance we could:
- open an issue for CUDA and add the link in some comment here
- insert a function
_sylvesterto avoid type piracy
There was a problem hiding this comment.
We should open an issue at AMDGPU also
|
Your PR no longer requires formatting changes. Thank you for your contribution! |
kshyatt
force-pushed
the
testsuite-ad
branch
2 times, most recently
from
0669d08 to
958dd36
Compare
kshyatt
force-pushed
the
testsuite-ad
branch
6 times, most recently
from
fad3f4a to
daeab4d
Compare
|
I think this is ready for review if CI passes, it's not testing the Diagonal stuff for now waiting on @lkdvos 's PR, but it will radically simplify adding tests for that and CUDA and Enzyme |
lkdvos
left a comment
lkdvos
left a comment
There was a problem hiding this comment.
I left a bunch of smaller comments through the code, and only quickly went through the actual test suites. Am I correct that these haven't changed much?
I think in general it would be nice to try and link some of the GPU issues we are facing with actual issue numbers and resolve them through dispatch alternatives, rather than having to alter the code for regular arrays as well.
In particular, isa(A, Array) seems a bit to strict since we also support views etc, which would then also take the slow path.
| L = kron(diagm(I_n), A) + kron(adjoint(B), diagm(I_m)) | ||
| x_vec = L \ -vec(C) | ||
| X = CuMatrix(reshape(x_vec, m, n))=# | ||
| hX = sylvester(collect(A), collect(B), collect(C)) |
There was a problem hiding this comment.
Is there any chance we could:
- open an issue for CUDA and add the link in some comment here
- insert a function
_sylvesterto avoid type piracy
| MatrixAlgebraKit.default_pullback_gauge_atol(A::AnyCuArray) = MatrixAlgebraKit.iszerotangent(A) ? 0 : eps(norm(CuArray(A), Inf))^(3 / 4) | ||
| function MatrixAlgebraKit.default_pullback_gauge_atol(A::AnyCuArray, As...) | ||
| As′ = filter(!MatrixAlgebraKit.iszerotangent, (A, As...)) | ||
| return isempty(As′) ? 0 : eps(norm(CuArray.(As′), Inf))^(3 / 4) |
There was a problem hiding this comment.
Why was this needed? what breaks if we don't do CuArray.(As')?
There was a problem hiding this comment.
norm doesn't work for Adjoint{CuArray} for example
There was a problem hiding this comment.
Does it help if we use the regular norm instead of the Inf one?
| :svd_compact, :svd_trunc, :svd_trunc_no_error, :svd_vals, | ||
| :left_polar, :right_polar, | ||
| ) | ||
| copy_f = Symbol(:cr_copy_, f) |
There was a problem hiding this comment.
Why is this cr_ ? what does that mean?
There was a problem hiding this comment.
I think I just got the difference with the mc_ ones, I thought this should all be run in separate modules though so there shouldn't necessarily be any nameclashing here?
I might be missing something though
There was a problem hiding this comment.
Yes they might be, I gave them different names just to be super clear but I guess it is not so clear 😓
Co-authored-by: Lukas Devos <[email protected]>
Co-authored-by: Lukas Devos <[email protected]>
Co-authored-by: Lukas Devos <[email protected]>
Co-authored-by: Lukas Devos <[email protected]>
Co-authored-by: Lukas Devos <[email protected]>
Co-authored-by: Lukas Devos <[email protected]>
Co-authored-by: Lukas Devos <[email protected]>
This lets us reuse a lot of the setup infrastructure for ChainRules, Mooncake, and (soon) Enzyme. Also starts testing the AD rules on GPU.