Skip to content

test(linalg): GPU correctness coverage for Kron and Outer (#1003, confirms #999)#1100

Closed
yingjerkao wants to merge 1 commit into
fix/outer-16bit-bool-dispatchfrom
test/gpu-kron-outer-coverage
Closed

test(linalg): GPU correctness coverage for Kron and Outer (#1003, confirms #999)#1100
yingjerkao wants to merge 1 commit into
fix/outer-16bit-bool-dispatchfrom
test/gpu-kron-outer-coverage

Conversation

@yingjerkao

Copy link
Copy Markdown
Collaborator

Problem

Kron and Outer had 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.cpp and Outer_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:

Inputs are built on the CPU and moved with .to(Device.cuda) (GPU arange ignores a non-unit step for real dtypes, #1070).

Testing

All 9 tests pass on an RTX 4070 Ti SUPER (CUDA 13.0, sm_89):

[  PASSED  ] 9 tests.   (GpuKron: 5, GpuOuter: 4)

Verified against a build with the parent commit's Outer_ii fix applied (the Outer cross-check's CPU reference needs the Int16/Uint16 dispatch rows). The cuKron/cuOuter/Kron.cpp/Outer.cpp sources under test are identical to origin/master.

Notes

Part of #1003. Confirms #999.

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>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +49 to +50
for (cytnx_uint64 i = 0; i < 4; i++)
for (cytnx_uint64 j = 0; j < 4; j++)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
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
  1. 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)

Comment on lines +77 to +79
const cytnx_uint64 bR = 2, bC = 3;
for (cytnx_uint64 i = 0; i < 6; i++)
for (cytnx_uint64 j = 0; j < 6; j++) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
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
  1. 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) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Avoid using legacy typedefs like cytnx_uint64 for lambda parameters representing indices in new code. Use standard C++ types like uint64_t instead.

Suggested change
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
  1. 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) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Avoid using legacy typedefs like cytnx_uint64 for lambda parameters representing indices in new code. Use standard C++ types like uint64_t instead.

Suggested change
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
  1. 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;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
const cytnx_double tol = (dtype == Type.ComplexFloat) ? 0.1 : 1e-6;
const double tol = (dtype == Type.ComplexFloat) ? 0.1 : 1e-6;
References
  1. 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)

Comment on lines +34 to +35
for (cytnx_uint64 i = 0; i < 3; i++)
for (cytnx_uint64 j = 0; j < 2; j++)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
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
  1. 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) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Avoid using legacy typedefs like cytnx_uint64 for lambda parameters representing indices in new code. Use standard C++ types like uint64_t instead.

Suggested change
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
  1. 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) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Avoid using legacy typedefs like cytnx_uint64 for lambda parameters representing indices in new code. Use standard C++ types like uint64_t instead.

Suggested change
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
  1. 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;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
const cytnx_double tol = (dtype == Type.ComplexFloat) ? 0.1 : 1e-6;
const double tol = (dtype == Type.ComplexFloat) ? 0.1 : 1e-6;
References
  1. 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)

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment on lines +122 to +123
b.at<cytnx_double>({0, 0}) = 2.0;
b.at<cytnx_double>({0, 1}) = -0.5;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

Comment on lines +72 to +73
b.at<cytnx_double>({0}) = 3;
b.at<cytnx_double>({1}) = 0.5;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

@yingjerkao

Copy link
Copy Markdown
Collaborator Author

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 OuterKron rewrite + GPU test files it depended on already landed on master via #1105/#1106 — so it can't merge as-is (it would try to re-apply changes already present and conflict).

But its tests were more thorough than what landed, so rather than lose them, #1108 folds this PR's stronger cases into master's existing tests/gpu/linalg_test/{Kron,Outer}_test.cpp:

  • 2-D hand-computed Kron/Outer (fractional + negative values),
  • complex × complex hand-computed cases,
  • the ComplexFloat × Double → ComplexDouble promotion discriminator,
  • the broad GPU-vs-CPU all-dtype differential (GpuMatchesCpuAllDtypes),

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!

@yingjerkao yingjerkao closed this Jul 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant