Open
Conversation
kshyatt
reviewed
Mar 25, 2026
kshyatt
previously approved these changes
Mar 25, 2026
Member
kshyatt
left a comment
kshyatt
left a comment
There was a problem hiding this comment.
Apart from minor comment LGTM, this will be very helpful for the Enzyme PR
Contributor
|
Your PR no longer requires formatting changes. Thank you for your contribution! |
Member
|
Ope, GPU failures look related... |
Member
Author
|
Needs #390 first, but should then be ready. |
Codecov Report✅ All modified and coverable lines are covered by tests.
... and 2 files with indirect coverage changes 🚀 New features to boost your workflow:
|
lkdvos
force-pushed
the
ld-testing
branch
2 times, most recently
from
ab125b5 to
e3ce090
Compare
Jutho
reviewed
Mar 30, 2026
| return (Vtr, Vℤ₃, VU₁, VfU₁, VCU₁, VSU₂, VfSU₂, VIB_diag, VIB_M) | ||
| end | ||
|
|
||
| # Gauge-fixing tangents for AD factorization tests |
Member
There was a problem hiding this comment.
Clearly we should have these functions in the main MAK module?
Member
Author
There was a problem hiding this comment.
I think this is in line with the current MAK approach. I'm happy to migrate this, but probably we should then also do this for MAK itself so we can reuse the functions.
Jutho
reviewed
Mar 30, 2026
Jutho
reviewed
Mar 30, 2026
Jutho
reviewed
Mar 30, 2026
Jutho
reviewed
Mar 30, 2026
Jutho
reviewed
Mar 30, 2026
Member
|
This generally looks good; I left a few small comments and questions. But clearly, this is too much change for a detailed review. Is there a convenient way to review such code reorganization, i.e. to separate between what has just moved to other files and what are actual changes. I could probably ask some agent, but I don't feel like doing that. |
Member
Author
|
I don't think there is, but I did try and not actually alter anything except for the organization of the tests.
In principle there is no reason to review the actual contents of the test files, since these are unchanged, which also explains some of the things you commented on. |
Member
|
That is quite likely yes; that file has a TODO to turn it into proper tests for quite some while already. |
Member
|
Ok, this took longer than anticipated, but locally all tests are passing. This does not include CUDA, so that can definitely still fail, or other Julia versions. I also had to disable some things such as chainrules for |
Member
|
Ok, I think I made the changes that I wanted here. I also synchronized the CUDA tests to be more in line with the CPU tensor and factorization tests in terms of tensors (with a little help from my friends However, without being able to run these locally, nor seeing the detailed progress report, this is very hard to debug. Do you know how to get more details out of logs of the aborted buildkite runs @kshyatt ? Or do you want to take a look. If we do not want to special case the multifusion cases, the only tensors that are valid are: "square" tensors (same domain or codomain), where the domain is either or "rectangular" tensors which are some "cyclic" permutations (i.e. transpositions) of So things like |
Member
|
Will definitely take a look though it may have to wait until after RVV ;)
…On Sun, Apr 5, 2026 at 1:32 PM Jutho ***@***.***> wrote:
*Jutho* left a comment (QuantumKitHub/TensorKit.jl#389)
<#389 (comment)>
Ok, I think I made the changes that I wanted here. I also synchronized the
CUDA tests to be more in line with the CPU tensor and factorization tests
in terms of tensors (with a little help from my friends [image:
 ]
) . However, even after increasing the buildkite workflow to a 90 minute
limit, they seem to time out (which is strange; these are not AD tests, the
corresponding CPU tests take about 10 minutes).
However, without being able to run these locally, nor seeing the detailed
progress report, this is very hard to debug. Do you know how to get more
details out of logs of the aborted buildkite runs @kshyatt
<https://github.com/kshyatt> ? Or do you want to take a look.
If we do not want to special case the multifusion cases, the only tensors
that are valid are:
"square" tensors (same domain or codomain), where the domain is either V1,
V1 * V2, V1 * V2 * V3, ... (in sequential order)
or
"rectangular" tensors which are some "cyclic" permutations (i.e.
transpositions) of V1 * V2 * V3 * V4 * V5 (all 5 are necessary).
So things like V1 * V2 * V3 ← (V4 * V5)' .
—
Reply to this email directly, view it on GitHub
<#389 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAGKJY3QANSRCNBJQCRWUU34UI75HAVCNFSM6AAAAACW6DFFAGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DCOBYG42DKNJUG4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
borisdevos
reviewed
Apr 6, 2026
|
|
||
| # Sector utility | ||
| # -------------- | ||
| smallset(::Type{I}) where {I <: Sector} = take(values(I), 5) |
Member
There was a problem hiding this comment.
Do we want to import the TensorKitSectors testsuite to have the same helper functions, or just manually carry them over here? Either way, I think it's useful to be using the same smallsets.
Member
|
I was able to run the CUDA tests locally, I'll try to fix them tomorrow. Here are the outputs: Looks like the new A_4 tests are absolutely wrecking everything on time... |
Member
|
Additional problem is the |
Member
Oh I removed that one because it wasn't working correctly. You want to ensure that, if you use The old @test abs(dim(domain(d)) - nvals) ≤ maximum(c -> blockdim(domain(t), c), blocksectors(t); init = 1)I thought I replaced them all, but apparently not. |
Member
|
A4Irrep is added to have |
Member
If that's the case, then would it make sense to try to get #393 in before this? Or not support the Also, I am somewhat dubious of the mixed cpu-gpu explanation as that should rather throw a There are only 4 cases in the norm preservation situation, let me see if I can narrow down where the problem is. |
Member
|
@Jutho some timing is showing that, for the norm preservation case, the vast vast majority of time is spent in the tensor product step. Since this is the first time the @timedtestset "Tensor product: test via norm preservation" begin
for T in (ComplexF64, ) # Float32 case broken because of cuTENSOR
@time "Construction" begin
if UnitStyle(I) isa SimpleUnit || !isempty(blocksectors(V2 ⊗ V1))
t1 = CUDA.rand(T, V2 ⊗ V3 ⊗ V1, V1 ⊗ V2)
t2 = CUDA.rand(T, V2 ⊗ V1 ⊗ V3, V1 ⊗ V1)
else
t1 = CUDA.rand(T, V3 ⊗ V4 ⊗ V5, (V1 ⊗ V2)')
t2 = CUDA.rand(T, (V3 ⊗ V4 ⊗ V5)', V1 ⊗ V2)
end
end
@time "Product" begin
t = @constinferred (t1 ⊗ t2)
end
@time "Norm" begin
@test norm(t) ≈ norm(t1) * norm(t2)
end
end
end
symmetricbraiding && @timedtestset "Tensor product: test via conversion" begin
for T in (Float32, ComplexF64)
t1 = CUDA.rand(T, V2 ⊗ V3 ⊗ V1, V1)
t2 = CUDA.rand(T, V2 ⊗ V1 ⊗ V3, V2)
d1 = dim(codomain(t1))
d2 = dim(codomain(t2))
d3 = dim(domain(t1))
d4 = dim(domain(t2))
t = @constinferred (t1 ⊗ t2)
At = ad(t)
@test ad(t) ≈ ad(t1) ⊗ ad(t2)
end
end For the top case, we have |
Member
This is something I noticed in the codecov PR, and I tried fixing with something like this. The spaces have shifted since, but it was something I saw locally already, and probably carries over to the GPU tests. |
Member
|
Indeed cutting the spaces a bit helped tremendously, however we're still being hit hard by the permutation tests which I will now look at. More and more I think getting the changes in #393 will help, if anyone wants to review it 🥺 |
Member
|
Not sure what happened with this: if UnitStyle(I) isa SimpleUnit || !isempty(blocksectors(V2 ⊗ V1))
t1 = CUDA.rand(T, V2 ⊗ V3 ⊗ V1, V1 ⊗ V2)
t2 = CUDA.rand(T, V2 ⊗ V1 ⊗ V3, V1 ⊗ V1)
else
t1 = CUDA.rand(T, V3 ⊗ V4 ⊗ V5, (V1 ⊗ V2)')
t2 = CUDA.rand(T, (V3 ⊗ V4 ⊗ V5)', V1 ⊗ V2)
end If this is something I have inserted, it might have been an oversight from something copilot generated. I was aiming to avoid all special casing, and so the only universally valid case is the |
Member
|
Copilot flew the tests directly into the side of the mountain! 😭 |
Member
|
Oh wait, I now see the same kind of constructions exist in the CPU tests. I did try to carefully check all copilot changes, so the reason I do not remember this is probably because it was like this from the before. I mainly focussed on the factorization tests, as all the tensor tests seemed fine. |
Member
|
I'll push a commit with some smaller tests. I did not run them locally, so let's hope for the best. |
Member
|
Buildkite now succeeding!!! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
test/Project.toml, separating them from the main package manifest--fastmode that skips AD test groups and reduces sector/scalar-type coverage for quick iterationtest/README.mddocumenting how to run tests, available groups, fast mode, and how to add new test files