Fix lanczos launch grid - #3109
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe patch updates the SPDX attribution and corrects CUDA grid-dimension calculations and kernel launch argument ordering in the Lanczos solver. ChangesLanczos CUDA corrections
Estimated code review effort: 1 (Trivial) | ~5 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
cpp/include/raft/sparse/solver/detail/lanczos.cuh (1)
160-166: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInclude
raft/util/integer_utils.hppdirectly.
lanczos.cuhusesraft::div_rounding_up_safebut does not include its defining header. Add the direct include.🤖 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/include/raft/sparse/solver/detail/lanczos.cuh` around lines 160 - 166, Add a direct include of raft/util/integer_utils.hpp in lanczos.cuh so the visible uses of raft::div_rounding_up_safe are backed by their defining header; leave the surrounding kernel launch and block calculations unchanged.
🤖 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/include/raft/sparse/solver/detail/lanczos.cuh`:
- Around line 160-166: Before the kernel launches in the Lanczos setup path, add
RAFT_EXPECTS validation that enforces n_components + 1 < ncv and rejects k >=
ncv, ensuring invalid dimensions cannot produce zero-block launches or
out-of-bounds writes in kernel_triangular_beta_k.
- Around line 162-168: Add immediate CUDA error checks after both visible kernel
launches in the surrounding Lanczos flow: insert
RAFT_CUDA_TRY(cudaGetLastError()) directly after the triangular beta kernel
launch and the preceding kernel launch, preserving the existing conditional
structure and launch behavior.
---
Nitpick comments:
In `@cpp/include/raft/sparse/solver/detail/lanczos.cuh`:
- Around line 160-166: Add a direct include of raft/util/integer_utils.hpp in
lanczos.cuh so the visible uses of raft::div_rounding_up_safe are backed by
their defining header; leave the surrounding kernel launch and block
calculations unchanged.
🪄 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: 7a885499-e115-4b9c-8dfd-55f2cd9dd8d9
📒 Files selected for processing (1)
cpp/include/raft/sparse/solver/detail/lanczos.cuh
divyegala
left a comment
There was a problem hiding this comment.
Could you please add a regression test?
|
I did want to add it, but apparently any possible configuration is valid - just slow (low residency) |
|
/merge |
Fix accidentally swapped block size and grid size arguments.
#3104 (comment)