Replace hardcoded Lanczos eigenvalue goldens with property-based checks#3086
Replace hardcoded Lanczos eigenvalue goldens with property-based checks#3086says1117 wants to merge 6 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
|
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
WalkthroughChangesLanczos 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 |
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.