Repository navigation
Conversation
…ilable linalg::Qr on a GPU tensor threw "QR decomposition on GPU not implemented without cuQuantum support!" in builds without cuQuantum, which also broke Rand_isometry and every Rsvd call that sketches. With cuQuantum, is_tau=true threw because cutensornetTensorQR cannot return tau. Add linalg_internal::cuQr (cuQr_internal.cu): reduced QR of a row-major M x N matrix with cusolverDn<t>geqrf + orgqr/ungqr for Float, Double, ComplexFloat and ComplexDouble, dispatched with std::visit on the typed GPU pointer. It factors an explicit column-major copy, extracts R as the upper triangle/trapezoid, and for wide input forms Q from the leading M x M block. tau follows the CPU path's convention (gelqf on the transposed layout): the geqrf scalars for real dtypes, their complex conjugates for complex dtypes. Dimensions that do not fit cuSOLVER's 32-bit int are rejected. Qr.cpp uses the new kernel when UNI_CUQUANTUM is undefined, and for is_tau=true in any build. The cuQuantum path for is_tau=false is unchanged. Tests: new tests/gpu/linalg_test/Qr_test.cpp checks Q^H Q = I, Q R = A, R upper triangular, shapes/dtypes/device, and Q, R, tau against a host Householder reference and hand-computed cases, for four dtypes and tall, square, wide, rank-deficient, zero, non-contiguous, integer and dense UniTensor inputs. The eight "#ifndef UNI_CUQUANTUM" skip guards on the QR and Rsvd GPU tests are removed. Refs #255, #1111, #1227. Co-Authored-By: Claude <noreply@anthropic.com>
For complex dtypes the tau returned by linalg::Qr is the complex conjugate of the LAPACK geqrf scalar, because the CPU path runs gelqf on the transposed (row-major) layout; the GPU path reproduces it. Also correct the description of the third output, which is the vector of scalar factors, not the reflectors. Co-Authored-By: Claude <noreply@anthropic.com>
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1cf29252dc
ℹ️ 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".
|
|
||
| } // namespace | ||
|
|
||
| void cuQr(const Tensor &in, Tensor &Q, Tensor &R, Tensor &tau) { |
There was a problem hiding this comment.
Use snake_case for the new GPU QR entry point
This new CUDA implementation introduces cuQr and uppercase parameter names Q and R, contrary to the new-identifier convention in AGENTS.md. Because these are new files rather than additions to a consistently named legacy file, the existing-file exception does not apply. Rename the entry point to cu_qr and its output parameters to lowercase, updating the declaration and caller together.
AGENTS.md reference: AGENTS.md:L73-L75
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Posted by Claude Code on behalf of @pcchen
Not changed in this PR. Every sibling kernel entry point in src/backend/linalg_internal_gpu/ is named cuXxx (cuAdd_dispatch, cuDet_internal_d, cuEigh_internal_cf, cuConj_inplace_dispatch, ...): 32 of the 36 headers in that directory declare cuXxx functions and none declares a cu_xxx one. A lone cu_qr next to them would be harder to find than cuQr, and Q / R are the names the caller in src/linalg/Qr.cpp and the QR literature use for these outputs. Renaming the GPU kernels to snake_case is better done for the whole directory at once than for one function here.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1231 +/- ##
=======================================
Coverage 69.43% 69.43%
=======================================
Files 209 209
Lines 22614 22614
Branches 72 72
=======================================
Hits 15701 15701
Misses 6891 6891
Partials 22 22
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
Qr on a GPU tensor no longer returns tau: is_tau=true raises an error in every GPU build, as it already did with cuQuantum. Builds with cuQuantum use cutensornetTensorQR for every GPU Qr again, so this branch does not change their behaviour. The cuSOLVER kernel keeps the geqrf scalars as an internal buffer for orgqr / ungqr. Tests: drop the tau checks, add a rejection test, and compare Q and R with the Householder reference only in builds without cuQuantum. Co-Authored-By: Claude <noreply@anthropic.com>
… and tests Dimensions, bounds and loop counters are ordinary sizes, not dtype-backed tensor scalars. Shape containers passed to the Tensor API stay std::vector<cytnx_uint64>. Co-Authored-By: Claude <noreply@anthropic.com>
Posted by Claude Code on behalf of @pcchen
Closes #255. Closes #1111. Refs #1227, #1232.
Builds on #1228 (merged): the six
RsvdGPU tests this un-skips run through thegesvdjpath that #1228 fixed. The audit of the other cuQuantum-gated operations that #1111 also asks for is in #1227, not in this PR.This is new numerical code and needs human review. Points for the reviewer are listed under "For review".
Problem
In a GPU build without cuQuantum,
linalg::Qron a GPU tensor throws "QR decomposition on GPU not implemented without cuQuantum support!". That also breakslinalg::Rand_isometryand everylinalg::Rsvdcall that sketches, and the dense, block and fermionicUniTensoroverloads.Fix
linalg_internal::cuQr(src/backend/linalg_internal_gpu/cuQr_internal.cu): reduced QR of a row-major M x N matrix withcusolverDn<t>geqrfandorgqr/ungqr, for Float, Double, ComplexFloat and ComplexDouble.geqrfon the full matrix, Q from the leading M x M block, which satisfies them >= n >= krequirement oforgqr/ungqr.tau) are an internal buffer betweengeqrfandorgqr/ungqrand are not returned.std::visiton the typed GPU pointer; no new*_iilookup table. Shared cuSOLVER handle,DeviceBuffer,checkCudaErrorson every status,devInfochecked aftergeqrfand afterorgqr/ungqr.intare rejected withcytnx_error_msg.src/linalg/Qr.cpp, GPU branch: withoutUNI_CUQUANTUMthe new kernel is used; withUNI_CUQUANTUMthe cuQuantum path is used, as before.Qr(..., is_tau=true)on a GPU tensor raises "Returning tau is not supported on GPU" in every GPU build. The CPU path still returnstau.#ifndef UNI_CUQUANTUMskip guards are removed (two intests/gpu/linalg_test/linalg_test.cpp, six intests/gpu/linalg_test/Rsvd_test.cpp).linalg::Qdron GPU still throws; out of scope.For review
tauon GPU. An earlier revision of this PR returnedtaufrom cuSOLVER (also in cuQuantum builds, whereis_tau=trueused to throw). That was removed: nothing in the repository consumestau, it is of no use without the Householder vectors, whichQrdoes not return, and its complex convention on the CPU is an accident of the implementation (linalg.Qr(..., is_tau=True)returns the conjugate of thegeqrfscalars for complex input #1232). Whetheris_taushould be removed from the API altogether is forlinalg.Qr(..., is_tau=True)returns the conjugate of thegeqrfscalars for complex input #1232.is_tau=trueerror, which is now raised before the factorization instead of after it.tauconvention is documented, not changed (include/linalg.hpp,QR_internal.cpp): for complex dtypes the CPU returns the complex conjugate of thegeqrfscalar, because it runsgelqfon the transposed layout.checkCudaErrors, which exits the process (CUDA error paths still terminate the process with exit() #1164), as in the other cuSOLVER wrappers. A nonzerodevInforaises a catchablecytnx::error.INT_MAXrejection is not covered by a test (it would need a matrix with more than 2^31 rows or columns).Testing
New
tests/gpu/linalg_test/Qr_test.cpp(11 tests). Expected values come from host code in the test file, not from another Cytnx path: a textbook Householder QR in the LAPACKgeqrfconvention, plain matrix products, and hand-computed cases.Checked in every build (invariants): Q^H Q = I, Q R = A, R exactly zero below the diagonal, shapes / dtype / device. Checked only in builds without cuQuantum (values): Q and R against the Householder reference and the hand-computed cases. cuQuantum builds run these tests through
cutensornetTensorQR, which does not document which of the valid QR factorizations it returns, so the two hand-computed tests are skipped there.UniTensorwith rowrank 1 and 2.is_tau=trueon a GPUTensorand on a GPU denseUniTensorraises.Suites run:
Qr*,Rsvd*,RsvdNoTruncation*,Gesvd*,GesvdTruncate*,Svd*,SvdTruncate*,linalg_Test*,ZeroExtent*.Current head (
f3a7df73), ran / passed / skipped / failed:master(c7fda0ad)masterSvd.GpuU1ZerosTestandlinalg_Test.GpuBkUtExpM, unrelated to QR. The 2 extra skips on the A100 are the hand-computed QR tests described above.linalg_Test.GpuBkUtSvdTruncate2. That is the intermittent hang reported in [gpu] Tensor get/set silently drops elements (and occasionally hangs) for selections over 2048 elements #1229 (fix in fix(gpu): copy get/set elements in-kernel instead of device-side cudaMemcpyAsync #1230, not inmasteryet); it also occurs without this change.Earlier revision of this PR (the one that returned
tau, on the older baseeade1422). The factorization code of the kernel is the same; these hosts were not rerun for the current head:<cuda/std/cstdlib>is missing from its bundled headers), so the P100 run used a CUDA 12.9 toolkit copied from the V100 host andgcc-toolset-13.Svd.*/SvdTruncate.*/linalg_Test.GpuDenseUtSvdTruncatetests before and after: a 2 GB hardware limit, not QR.Qr.cppfrom the base): on the V100, the P100 and the laptop 19 tests failed with the "not implemented without cuQuantum" error (the 11Qr.*tests of that revision, 2linalg_TestQR tests, 6Rsvd).Qragainst GPU cuSOLVERQr(temporary probe, not committed), 4 dtypes x 5 shapes up to 96x64, V100 and P100: Q and R agree to rounding (largest difference 1.1e-14 in double, 8.0e-6 in single precision, both on R of the 64x96 case).Not run locally: the CPU
test_mainsuite andpytest(CPU code is changed in comments only; CI runs both), the full GPU suite outside the listed suites, and multi-GPU.🤖 Generated with Claude Code