test(linalg): GPU correctness coverage for Kron and Outer (#1003, confirms #999)#1100
test(linalg): GPU correctness coverage for Kron and Outer (#1003, confirms #999)#1100yingjerkao wants to merge 1 commit into
Conversation
Kron and Outer had no GPU tests. Add tests/gpu/linalg_test/Kron_test.cpp and Outer_test.cpp exercising the GPU paths with independently hand-computed expected values (not GPU-vs-CPU as the primary oracle): - Double and ComplexDouble with literal and per-element-recomputed results (Kron also covers fractional/negative values); - the mixed ComplexFloat x Double -> ComplexDouble promotion discriminator (output dtype AND value), the case that motivated #999 / #984 -- the product must be computed and stored through the promoted output type; - a broad GPU-vs-CPU cross-check over every real/complex dtype as secondary coverage (ComplexFloat gets a looser tolerance because ~1e6-magnitude float32 complex products diverge from the CPU one at the ULP; a single real multiply is bit-identical). Inputs are built on the CPU and moved with .to(Device.cuda) since GPU arange ignores a non-unit step for real dtypes (#1070). The Outer cross-check's CPU reference relies on the Int16/Uint16 Outer dispatch fix in the parent commit; without it those dtypes crash. Testing: all 9 tests pass on an RTX 4070 Ti SUPER (CUDA 13.0, sm_89). This also confirms #999 (the GPU library builds clean and Kron is numerically correct). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request adds GPU test suites for the Kronecker product (Kron) and outer product (Outer) linear algebra operations, verifying correctness across various data types, fractional/negative values, complex numbers, and type promotions. The reviewer feedback focuses on adhering to the repository's style guide by replacing legacy cytnx_XXXX typedefs (such as cytnx_uint64 and cytnx_double) with standard C++ types (like uint64_t and double) for loop counters, local variables, and lambda parameters in the newly added test files.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| for (cytnx_uint64 i = 0; i < 4; i++) | ||
| for (cytnx_uint64 j = 0; j < 4; j++) |
There was a problem hiding this comment.
Avoid using legacy typedefs like cytnx_uint64 for ordinary loop counters in new code. Use standard C++ types such as uint64_t or size_t instead.
| for (cytnx_uint64 i = 0; i < 4; i++) | |
| for (cytnx_uint64 j = 0; j < 4; j++) | |
| for (uint64_t i = 0; i < 4; i++) | |
| for (uint64_t j = 0; j < 4; j++) |
References
- Avoid propagating legacy cytnx_XXXX typedefs as ordinary loop counters, sizes, flags, or local arithmetic types in new or touched code. Prefer standard C++ types. (link)
| const cytnx_uint64 bR = 2, bC = 3; | ||
| for (cytnx_uint64 i = 0; i < 6; i++) | ||
| for (cytnx_uint64 j = 0; j < 6; j++) { |
There was a problem hiding this comment.
Avoid using legacy typedefs like cytnx_uint64 for local sizes and loop counters in new code. Standard C++ types like uint64_t or size_t are preferred.
| const cytnx_uint64 bR = 2, bC = 3; | |
| for (cytnx_uint64 i = 0; i < 6; i++) | |
| for (cytnx_uint64 j = 0; j < 6; j++) { | |
| const uint64_t bR = 2, bC = 3; | |
| for (uint64_t i = 0; i < 6; i++) | |
| for (uint64_t j = 0; j < 6; j++) { |
References
- Avoid propagating legacy cytnx_XXXX typedefs as ordinary loop counters, sizes, flags, or local arithmetic types in new or touched code. Prefer standard C++ types. (link)
| // out[i,j] = a[0, j] * b[i, 0] | ||
| // (1+1i)*(0+1i) = -1+1i ; (2-1i)*(0+1i) = 1+2i | ||
| // (1+1i)*(3+0i) = 3+3i ; (2-1i)*(3+0i) = 6-3i | ||
| auto near = [&](cytnx_uint64 i, cytnx_uint64 j, double re, double im) { |
There was a problem hiding this comment.
Avoid using legacy typedefs like cytnx_uint64 for lambda parameters representing indices in new code. Use standard C++ types like uint64_t instead.
| auto near = [&](cytnx_uint64 i, cytnx_uint64 j, double re, double im) { | |
| auto near = [&](uint64_t i, uint64_t j, double re, double im) { |
References
- Avoid propagating legacy cytnx_XXXX typedefs as ordinary loop counters, sizes, flags, or local arithmetic types in new or touched code. Prefer standard C++ types. (link)
| ASSERT_EQ(out.dtype(), Type.ComplexDouble); | ||
| ASSERT_EQ(out.shape(), (std::vector<cytnx_uint64>{2, 2})); | ||
| // out[i,j] = a[i,0] * b[0,j] | ||
| auto near = [&](cytnx_uint64 i, cytnx_uint64 j, double re, double im) { |
There was a problem hiding this comment.
Avoid using legacy typedefs like cytnx_uint64 for lambda parameters representing indices in new code. Use standard C++ types like uint64_t instead.
| auto near = [&](cytnx_uint64 i, cytnx_uint64 j, double re, double im) { | |
| auto near = [&](uint64_t i, uint64_t j, double re, double im) { |
References
- Avoid propagating legacy cytnx_XXXX typedefs as ordinary loop counters, sizes, flags, or local arithmetic types in new or touched code. Prefer standard C++ types. (link)
| if (dtype == Type.Bool) continue; // Kron has no Bool kernel | ||
| // See Outer_test's note: ~1e6-magnitude complex-float products diverge | ||
| // from the CPU at the float32 ULP, so ComplexFloat gets a looser tol. | ||
| const cytnx_double tol = (dtype == Type.ComplexFloat) ? 0.1 : 1e-6; |
There was a problem hiding this comment.
Avoid using legacy typedefs like cytnx_double for local arithmetic types such as tolerance variables in new code. Use standard C++ types like double instead.
| const cytnx_double tol = (dtype == Type.ComplexFloat) ? 0.1 : 1e-6; | |
| const double tol = (dtype == Type.ComplexFloat) ? 0.1 : 1e-6; |
References
- Avoid propagating legacy cytnx_XXXX typedefs as ordinary loop counters, sizes, flags, or local arithmetic types in new or touched code. Prefer standard C++ types. (link)
| for (cytnx_uint64 i = 0; i < 3; i++) | ||
| for (cytnx_uint64 j = 0; j < 2; j++) |
There was a problem hiding this comment.
Avoid using legacy typedefs like cytnx_uint64 for ordinary loop counters in new code. Use standard C++ types such as uint64_t or size_t instead.
| for (cytnx_uint64 i = 0; i < 3; i++) | |
| for (cytnx_uint64 j = 0; j < 2; j++) | |
| for (uint64_t i = 0; i < 3; i++) | |
| for (uint64_t j = 0; j < 2; j++) |
References
- Avoid propagating legacy cytnx_XXXX typedefs as ordinary loop counters, sizes, flags, or local arithmetic types in new or touched code. Prefer standard C++ types. (link)
| ASSERT_EQ(out.shape(), (std::vector<cytnx_uint64>{2, 2})); | ||
| // (1+2i)*(0-1i) = 2-1i ; (1+2i)*(2+2i) = -2+6i | ||
| // (-3+1i)*(0-1i)= 1+3i ; (-3+1i)*(2+2i)= -8-4i | ||
| auto near = [&](cytnx_uint64 i, cytnx_uint64 j, double re, double im) { |
There was a problem hiding this comment.
Avoid using legacy typedefs like cytnx_uint64 for lambda parameters representing indices in new code. Use standard C++ types like uint64_t instead.
| auto near = [&](cytnx_uint64 i, cytnx_uint64 j, double re, double im) { | |
| auto near = [&](uint64_t i, uint64_t j, double re, double im) { |
References
- Avoid propagating legacy cytnx_XXXX typedefs as ordinary loop counters, sizes, flags, or local arithmetic types in new or touched code. Prefer standard C++ types. (link)
|
|
||
| ASSERT_EQ(out.dtype(), Type.ComplexDouble); | ||
| ASSERT_EQ(out.shape(), (std::vector<cytnx_uint64>{2, 2})); | ||
| auto near = [&](cytnx_uint64 i, cytnx_uint64 j, double re, double im) { |
There was a problem hiding this comment.
Avoid using legacy typedefs like cytnx_uint64 for lambda parameters representing indices in new code. Use standard C++ types like uint64_t instead.
| auto near = [&](cytnx_uint64 i, cytnx_uint64 j, double re, double im) { | |
| auto near = [&](uint64_t i, uint64_t j, double re, double im) { |
References
- Avoid propagating legacy cytnx_XXXX typedefs as ordinary loop counters, sizes, flags, or local arithmetic types in new or touched code. Prefer standard C++ types. (link)
| for (auto dtype : dtype_list) { | ||
| if (dtype == Type.Bool) continue; // Outer has no Bool kernel | ||
| SCOPED_TRACE("dtype " + std::to_string(dtype)); | ||
| const cytnx_double tol = (dtype == Type.ComplexFloat) ? 0.1 : 1e-6; |
There was a problem hiding this comment.
Avoid using legacy typedefs like cytnx_double for local arithmetic types such as tolerance variables in new code. Use standard C++ types like double instead.
| const cytnx_double tol = (dtype == Type.ComplexFloat) ? 0.1 : 1e-6; | |
| const double tol = (dtype == Type.ComplexFloat) ? 0.1 : 1e-6; |
References
- Avoid propagating legacy cytnx_XXXX typedefs as ordinary loop counters, sizes, flags, or local arithmetic types in new or touched code. Prefer standard C++ types. (link)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 19fc8688bf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| b.at<cytnx_double>({0, 0}) = 2.0; | ||
| b.at<cytnx_double>({0, 1}) = -0.5; |
There was a problem hiding this comment.
Use precision-sensitive doubles in this discriminator
In this mixed-promotion regression test, the Double operands are exactly representable as float, and the expected tolerance is loose enough that an implementation which still narrows the multiply to complex64 before producing a ComplexDouble result would pass with these values. That leaves the stated “full double precision” behavior untested for the ComplexFloat × Double path; use a non-float-representable double or a magnitude/cancellation case whose result changes after float narrowing.
Useful? React with 👍 / 👎.
| b.at<cytnx_double>({0}) = 3; | ||
| b.at<cytnx_double>({1}) = 0.5; |
There was a problem hiding this comment.
Use precision-sensitive doubles in this discriminator
This mixed-promotion test also uses Double values that are exactly representable as float, so it only verifies the dtype and not the promised full-double computation path. If the GPU path accidentally multiplies in complex64 and then writes/casts to ComplexDouble, these expected values still pass within 1e-6; choose a non-float-representable double value or another case whose product differs after float narrowing.
Useful? React with 👍 / 👎.
|
Closing — its GPU test coverage has been salvaged into #1108, and the rest is superseded by #1105/#1106. Background: this PR was stacked on the (now-closed) #1099 branch, and the But its tests were more thorough than what landed, so rather than lose them, #1108 folds this PR's stronger cases into
while keeping master's Int16 hand-computed Kron and the #1099 Int16-diagonal Outer segfault guard. Verified on GPU (CUDA 13, RTX 4070 Ti): 11/11 pass. Thanks! |
Problem
KronandOuterhad no GPU tests — their CUDA paths (cuKron_internal.cuh,cuOuter_internal.cu) were entirely uncovered. This is the coverage the #1003 Outer/Kron typed-dispatch consolidation needs as a safety net, and it also confirms #999 is resolved.Fix
Add
tests/gpu/linalg_test/Kron_test.cppandOuter_test.cpp. Expected values are computed independently from the operation definition (literals or per-element recompute), not by comparing against another Cytnx path that could share the same bug:ComplexFloat × Double → ComplexDouble— the promotion discriminator behind GPU build broken: cuKron fails to compile (CUDA 13) after the #982 type_promote change #999/fix(linalg): replace lower-enum-index dtype selection with Type.type_promote #984: the product must be computed and stored through the promoted output type. Checks output dtype and value.ComplexFloatuses a looser tolerance because ~1e6-magnitude float32 complex products diverge from the CPU at the ULP; a single real multiply is bit-identical.Inputs are built on the CPU and moved with
.to(Device.cuda)(GPUarangeignores a non-unit step for real dtypes, #1070).Testing
All 9 tests pass on an RTX 4070 Ti SUPER (CUDA 13.0,
sm_89):Verified against a build with the parent commit's
Outer_iifix applied (the Outer cross-check's CPU reference needs the Int16/Uint16 dispatch rows). ThecuKron/cuOuter/Kron.cpp/Outer.cppsources under test are identical toorigin/master.Notes
OuterInt16/Uint16/Bool dispatch fix). The Outer GPU cross-check's CPU reference crashes without it. Will retarget tomasteronce fix(linalg): register missing Int16/Uint16/Bool rows in CPU Outer dispatch (fixes segfault) #1099 merges.USE_CUDAbuild (as done here).Part of #1003. Confirms #999.