Forward rules for QR/LQ - #283
Conversation
|
Mooncake + CUDA is failing for a Mooncake internal reason I'll look into, will also look at the 1.10 Enzyme fails |
Jutho
left a comment
Jutho
left a comment
There was a problem hiding this comment.
Thanks, this looks very good. I left a few suggestions, but I think this is more or less good to go. I might still go through the test files in a bit more detail, but I like the approach to focus on gauge-invariant quantities.
|
The Mooncake PR needs to be merged for 1.10 + CUDA to pass, in the meantime I'll try all the suggestions |
Co-authored-by: Jutho <Jutho@users.noreply.github.com>
|
This has passed for everything except AMD, which doesn't run AD tests at all, and no longer has a |
| test_reverse(call_and_zero!, RT, (left_orth!, Const), (A, TA), (alg, Const); atol, rtol, fdm, output_tangent = ΔVC) | ||
| test_reverse(call_and_zero!, RT, (left_orth!, Const), (copy(A), TA), (alg, Const); atol, rtol, fdm, output_tangent = ΔVC) | ||
| test_forward(left_orth, RT, (A, TA), (alg, Const); atol, rtol, fdm) | ||
| test_forward(call_and_zero!, RT, (left_orth!, Const), (copy(A), TA), (alg, Const); atol, rtol, fdm) |
There was a problem hiding this comment.
Just to understand correctly: before we did not copy(A) in the test_reverse(call_and_zero!,...) because it was the final call, and we didn't care that A was destroyed afterwards. By adding test_forward tests, we still need A, so you copy(A) in test_reverse(call_and_zero!,...). However, is copy(A) strictly necessary test_forward(call_and_zero!,...), or is this as a hygiene measure or a safeguard when adding further tests later?
There was a problem hiding this comment.
It's a hygiene guard indeed
|
Nothing major. I do keep wondering if the case |
Pull request was closed
|
What the heck happened here lol |
|
Github being Github, I tried to merge this and just could not get it to recognize that it was actually on top of main, and closing/opening seemed to fix that |
Codecov Report❌ Patch coverage is
... and 25 files with indirect coverage changes 🚀 New features to boost your workflow:
|
3 participants
I ended up deciding to just zero out the "extra" spaces, and test the ones that are well-defined. These are the last factorizations we're missing forward rules for, I think.