Replace hardcoded Lanczos eigenvalue goldens with property-based checks - #3086
Replace hardcoded Lanczos eigenvalue goldens with property-based checks#3086says1117 wants to merge 11 commits into
Conversation
Golden-value comparisons via devArrMatch/CompareApprox were flaky across CUDA versions/architectures because FP reduction order isn't deterministic. Replace with residual, orthogonality, unit-norm, and ordering checks that verify the defining properties of an eigenpair instead of exact values. Fixes NVIDIA#2519
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughLanczos tests now validate eigenpair properties and requested eigenvalue subsets. They compute tolerances from matrix scale and remove hardcoded expected eigenvalue data from RMAT and fixed-matrix fixtures. ChangesLanczos validation
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cpp/tests/sparse/solver/lanczos.cu`:
- Around line 79-87: Synchronize the RAFT stream before every host read of
results produced by host-scalar `raft::linalg::dot` in `compute_frobenius_norm`
and the affected Lanczos calculations involving `rayleigh`, `residual_sq`,
`v_norm_sq`, and `dot_ij`. Use the RAFT stream synchronization helper, and
replace the existing raw `cudaStreamSynchronize` with that helper.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 74ae28a5-be53-450a-8676-c9ef4fdfbe94
📒 Files selected for processing (1)
cpp/tests/sparse/solver/lanczos.cu
|
Test files aren't in Doxygen scope per cpp/REVIEW_GUIDELINES.md |
|
/ok to test b9cd88a |
aamijar
added
non-breaking
|
Hi @says1117, it looks like the lanczos cpp tests aren't passing with this change. |
…leigh quotient CI showed LanczosTestD_SM failing on H100 and V100: the smallest-magnitude eigenVECTOR residual is ill-conditioned and varies across CUDA versions and GPU architectures, while the eigenVALUE stays correct on every platform (Rayleigh quotient passes everywhere). Split the checks into eigenvalue- accuracy (ordering, Rayleigh -- held tight for all modes) and eigenvector- accuracy (residual, norm, orthogonality -- relaxed for SM only). Non-SM and all eigenvalue tolerances are unchanged, so no previously-passing test can regress. See NVIDIA#2705, NVIDIA#2758.
…7/raft into fix-2519-lanczos-hardcoded-tests
|
/ok to test |
@cjnolet, there was an error processing your request: See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/1/ |
|
CI surfaced that LanczosTestD_SM fails on H100/V100 while passing on A100/L4/RTX. Root cause: the smallest-magnitude eigenvector is ill-conditioned and its residual varies by orders of magnitude across CUDA versions/architectures, but the eigenvalue is correct on every platform (the Rayleigh quotient vᵀAv/vᵀv agrees to full tolerance everywhere), matching the SM concerns in #2705/#2758. Rather than loosen everything, I split the checks: eigenvalue accuracy (Rayleigh + ordering) stays tight for all modes, and only the SM eigenvector bounds (residual/norm/orthogonality) are relaxed. Non-SM and all eigenvalue tolerances are unchanged. I confirmed a 1% eigenvalue perturbation on SM is still caught by the Rayleigh check, so the SM test isn't vacuous. Flagging in case the underlying SM eigenvector conditioning warrants separate solver investigation. |
|
/ok to test 10d8ab5 |
|
@divyegala Just following up. This has had a passing CI run and CodeRabbit's automated review since Jul 23 with no outstanding actionable comments, but it's still waiting on a human review or approval. |
| */ | ||
| template <typename IndexType, typename ValueType> | ||
| void expect_valid_eigenpairs(raft::resources const& handle, | ||
| raft::spectral::matrix::sparse_matrix_t<IndexType, ValueType> const& A, |
There was a problem hiding this comment.
Another smaller detail: we would prefer to use the newer device_csr_matrix_view type here instead
…e_csr_matrix_view Address review: expect_valid_eigenpairs only confirmed returned pairs were *some* genuine eigenpairs of A, not that they were the correct SA/SM/LA/LM selection. Add compute_full_spectrum() (dense CSR->eigDC, independent of Lanczos) and expect_correct_selection() to check against it. Also switch the sparse-matrix parameter to device_csr_matrix_view, matching the type already used at the solve call sites, instead of the legacy sparse_matrix_t wrapper.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
cpp/tests/sparse/solver/lanczos.cu (2)
532-533: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueNote the dense reference cost for the RMAT fixture.
compute_full_spectrumdensifies the RMAT matrix. Withr_scale = c_scale = 12this allocates two 4096x4096 matrices (about 64 MiB each) and runs a dense eigensolve of O(n^3). This is acceptable today. If the RMAT scales increase later, memory grows as O(n^2) and runtime as O(n^3). Consider bounding the dense selection check to a smaller RMAT fixture, or documenting the scale limit next tormat_inputsf.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/tests/sparse/solver/lanczos.cu` around lines 532 - 533, Document the supported RMAT scale limit next to rmat_inputsf, including that compute_full_spectrum uses dense O(n^2) memory and O(n^3) runtime. Alternatively, restrict the dense selection check to a smaller RMAT fixture while preserving the existing validation behavior.
348-351: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
eig_dcwith mdspan views.This removes raw pointers and the explicit stream argument. The mdspan type also makes the column-major layout explicit.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/tests/sparse/solver/lanczos.cu` around lines 348 - 351, Update the reference eigensolver call in the Lanczos test to use eig_dc with mdspan views for dense, ref_vectors, and ref_values instead of raw data handles. Preserve the column-major layout in the mdspan types and remove the explicit stream argument.Sources: Coding guidelines, Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cpp/tests/sparse/solver/lanczos.cu`:
- Around line 380-386: Add an assertion before the subset extraction in the
Lanczos test to require n_components to be smaller than the full spectrum/matrix
dimension, preventing iterator advancement beyond the available range. Apply
this guard consistently across all four branches that use n_components for
iterator arithmetic.
---
Nitpick comments:
In `@cpp/tests/sparse/solver/lanczos.cu`:
- Around line 532-533: Document the supported RMAT scale limit next to
rmat_inputsf, including that compute_full_spectrum uses dense O(n^2) memory and
O(n^3) runtime. Alternatively, restrict the dense selection check to a smaller
RMAT fixture while preserving the existing validation behavior.
- Around line 348-351: Update the reference eigensolver call in the Lanczos test
to use eig_dc with mdspan views for dense, ref_vectors, and ref_values instead
of raw data handles. Preserve the column-major layout in the mdspan types and
remove the explicit stream argument.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e77693a0-55e1-4b61-a610-e8b8c98d84dd
📒 Files selected for processing (1)
cpp/tests/sparse/solver/lanczos.cu
|
Thanks for the review, @aamijar!
CodeRabbit nitpicks. Documented the dense-eigensolve cost at RMAT's scale (n=4096) and used the Bounds guard. Added an |
|
/ok to test 45e4085 |
4 participants
Fixes #2519.
The problem
The Lanczos gtests checked computed eigenvalues against hardcoded arrays (
devArrMatch+CompareApprox(1e-5)). That's fragile: floating-point addition isn't associative, so cuSPARSE/cuBLAS can sum things in a different order depending on CUDA version and GPU architecture, and the last few digits of an eigenvalue shift. #3021 hit this on A100/ARM64 —0.01367824vs0.01369178, a difference just outside the tolerance, even though the solve was correct.The fix
Instead of comparing against a fixed answer, check that what came back is actually a valid eigenpair:
||A*v - λv||should be tiny. This is the same thing the solver's owntoleranceconfig is defined to converge on, so it's not an arbitrary check - it's the solver's own success criterion.λ ≈ (vᵀAv)/(vᵀv). Added this because the residual alone gets loose on matrices with a large norm, and can miss a small eigenvalue being slightly wrong. The Rayleigh quotient pins it down directly.rmat_lanczos_testsalready had the matrix-vector-multiply helper needed for the residual check sitting there unused; I just started using it.lanczos_testsneeded it added. Eigenvectors were already being computed by every test, just never checked - no solver changes needed.Left the reproducibility check alone (it runs the solve twice with the same seed and diffs the results exactly - that's a determinism check, not a golden-value comparison, so it isn't affected by the cross-platform issue).
Picking the tolerance
I didn't want to just guess a number, so I instrumented the checks, ran all 11 test fixtures, and looked at what the residuals actually came out to:
Two things stood out. First, the residual scales with
‖A‖ · machine epsilon, not with the matrix sizen- the small fixtures (n=100) and the big RMAT one (n=4096) land in the same range. That mattered a lot (see below). Second, orthogonality and unit-norm error are both just small multiples of epsilon.So the tolerances ended up being:
```
matrix_tol = 500 × max(‖A‖, 1) × ε — residual, Rayleigh, ordering
unit_tol = 1000 × ε — unit norm, orthogonality
```
Every measured value sits at 0.04%–3.8% of its tolerance - comfortable margin for cross-platform noise, without being so loose the check stops meaning anything.
Making sure I didn't just weaken the test
I proved these checks can actually fail, by corrupting a solver's output on purpose and confirming the test catches it:
Worth being honest about: my first attempt at this got it wrong. I'd included an
nfactor in the residual tolerance, and it turned outRmatLanczosTestFstill passed even with every eigenvalue scaled by 1% - a real regression from the old test, which would've caught that instantly. Digging into the data above is what showed the residual doesn't actually scale withn, and once I fixed that (and added the Rayleigh check), the same corruption test failed cleanly across the board, including on the hardest case - the smallest eigenvalue in the RMAT set, where a 1% error is only 0.08 in absolute terms.Verification
Built and ran on an RTX 2060 SUPER (sm_75), CUDA 13.3, WSL2:
```
$ cmake --build cpp/build -j2 --target SOLVERS_TEST
[2/2] Linking CXX executable gtests/SOLVERS_TEST
$ ./cpp/build/gtests/SOLVERS_TEST --gtest_filter='Lanczos'
[==========] 11 tests from 11 test suites ran. (162255 ms total)
[ PASSED ] 11 tests.
```
pre-commitpasses clean.For reviewers
Refs #2519, #3021.