Add sparse Lanczos SVD solver#3034
Conversation
aamijar
added
non-breaking
|
Hi @Intron7, I haven't looked at this PR closely yet, but one thing to think about is that we should try and reuse parts of the existing lanczos eigensolver as much as possible. They should have some things in common right? |
|
/ok to test eabeaa4 |
|
@aamijar the algorithm is different even is the name is similar. The two paths share the Lanczos name but the kernels are different algorithms: the eigensolver builds a symmetric tridiagonal via a one-vector recurrence and ritz-solves with syevd; the SVD builds a bidiagonal via Golub-Kahan with two coupled bases (A @ v and Aᵀ @ u) and ritz-solves with gesvdj. Restart is also different, the SVD path locks converged singular triplets and restarts on the unconverged subspace. |
|
/ok to test b80ca30 |
|
/ok to test 93e9a19 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a Lanczos bidiagonalization SVD solver for sparse matrices in RAFT, including detail implementation, public/runtime C++ APIs, shared solver config types (replacing svds_config.hpp), CMake wiring, benchmarks, C++ tests, Python bindings for ChangesSparse Lanczos SVD solver
Estimated code review effort: 4 (Complex) | ~75 minutes Related issues: None specified. Related PRs: None specified. Suggested labels: cpp, python, feature request, non-breaking Suggested reviewers: cjnolet, tfeher 🐇 A lanczos hop, bidiagonal and spry,Restarts and locks singular triplets nearby, CGS or MGS2, whichever you choose, Benchmarks and tests all wired to amuse, Python calls too — the rabbit says "svd, aye!" 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 2
🧹 Nitpick comments (6)
python/pylibraft/pylibraft/tests/test_sparse.py (1)
228-405: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer standalone test functions over class methods.
The new Lanczos tests are added as methods on
class TestSvds. The repo guideline asks for standalonetest_foo_bar()functions rather thanclass TestFootest classes. These new tests follow the file's existing class convention, so this is non-blocking, but consider extracting them (or the whole suite) into module-level functions in a follow-up.As per coding guidelines: "Write tests as standalone
test_foo_bar()functions rather thanclass TestFootest classes."🤖 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 `@python/pylibraft/pylibraft/tests/test_sparse.py` around lines 228 - 405, The new Lanczos tests are implemented as methods on TestSvds, but the test guideline prefers standalone test_foo_bar() functions. Move these cases out of the class and make them module-level tests, preserving the same assertions and parameters in functions like test_lanczos_diagonal_spectrum, test_lanczos_return_singular_vectors_modes, test_lanczos_rotated_clustered_spectrum, test_lanczos_rank_deficient_and_edge_shapes, test_lanczos_non_convergence_raises, and test_invalid_solver.Source: Coding guidelines
cpp/include/raft/sparse/solver/detail/lanczos_svds.cuh (2)
251-254: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winBreakdown epsilon is not type-aware.
eps = 1e-9is fixed regardless ofValueTypeT, butfloat's machine epsilon (~1.19e-7) is larger than this threshold, so breakdown detection is comparatively looser/less reliable for thefloatinstantiation than fordouble.🔢 Scale epsilon by machine precision
- ValueTypeT eps = ValueTypeT(1e-9); + ValueTypeT eps = std::sqrt(std::numeric_limits<ValueTypeT>::epsilon());🤖 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_svds.cuh` around lines 251 - 254, The breakdown threshold in lanczos_svds.cuh is hardcoded in the Lanczos SVD setup, so it is not scaled to ValueTypeT and is too small for float. Update the epsilon initialization near the op.rows()/op.cols() setup to derive the threshold from machine precision for the current ValueTypeT (for example via std::numeric_limits<ValueTypeT>::epsilon() with an appropriate scale) so the breakdown check in the Lanczos routine behaves consistently for both float and double.
40-50: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftRedundant host-device sync in the hot Lanczos step loop.
cublasnrm2in the default host pointer mode already synchronizes internally to populateresult; the followingresource::sync_stream(handle, stream)on Line 48 is a second blocking barrier. Combined with the fact thatnormalize_or_randomize(and thusvector_norm) is invoked twice per Lanczos step (Lines 296-306, 320-331), this yields up to2 * active_ncvexplicit host syncs per restart iteration, which can materially hurt performance on large sparse matrices in this hot path.⚡ Remove the redundant explicit sync
template <typename ValueTypeT> ValueTypeT vector_norm(raft::resources const& handle, ValueTypeT const* x, int n) { common::nvtx::range<common::nvtx::domain::raft> scope("lanczos_svds::vector_norm"); auto cublas_handle = resource::get_cublas_handle(handle); auto stream = resource::get_cuda_stream(handle); ValueTypeT result{}; RAFT_CUBLAS_TRY(raft::linalg::detail::cublasnrm2(cublas_handle, n, x, 1, &result, stream)); - resource::sync_stream(handle, stream); return result; }If the extra sync was added deliberately to guard against non-default cuBLAS stream/pointer-mode configurations, please confirm with a comment; otherwise this is safe to drop.
Also applies to: 192-215, 275-332
🤖 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_svds.cuh` around lines 40 - 50, The `vector_norm` helper in `lanczos_svds.cuh` is adding a redundant blocking `resource::sync_stream(handle, stream)` after `raft::linalg::detail::cublasnrm2`, which already completes before `result` is read in the default host pointer mode. Remove that extra sync from `vector_norm` and keep the rest of the Lanczos flow (`normalize_or_randomize` call sites) unchanged unless there is a documented need for a non-default cuBLAS mode; if so, add a brief comment near `vector_norm` explaining the requirement.Source: Coding guidelines
cpp/bench/prims/sparse/svds.cu (1)
312-371: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate benchmark classes differ by a single config line.
lanczos_svds_benchandlanczos_mgs2_svds_benchare otherwise identical. Consider parameterizing a single class with a constructor/bool flag instead of duplicating the fullrun_benchmarkbody.♻️ Proposed consolidation
template <typename value_t> -class lanczos_svds_bench : public svds_bench_base<value_t> { +class lanczos_svds_bench : public svds_bench_base<value_t> { public: - using svds_bench_base<value_t>::svds_bench_base; + explicit lanczos_svds_bench(svds_input const& p, bool use_mgs2 = false) + : svds_bench_base<value_t>(p), use_mgs2_(use_mgs2) + { + } void run_benchmark(::benchmark::State& state) override { auto csr_matrix = this->csr_view(); raft::sparse::solver::sparse_lanczos_svd_config<value_t> config; config.n_components = this->params.k; config.ncv = this->params.ncv; config.tolerance = value_t(1e-4); config.max_iterations = this->params.max_iterations; config.seed = 1234; + config.use_mgs2_orthogonalization = use_mgs2_; this->loop_on_state( state, [&]() { raft::sparse::solver::sparse_lanczos_svd(this->handle, config, csr_matrix, this->singular_values.view(), this->U.view(), this->Vt.view()); }, false); } + + private: + bool use_mgs2_; }; - -template <typename value_t> -class lanczos_mgs2_svds_bench : public svds_bench_base<value_t> { - public: - using svds_bench_base<value_t>::svds_bench_base; - ... -};🤖 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/bench/prims/sparse/svds.cu` around lines 312 - 371, Consolidate the duplicated benchmark logic in lanczos_svds_bench and lanczos_mgs2_svds_bench by extracting the shared run_benchmark flow into one implementation and parameterizing the orthogonalization choice with a constructor flag or similar field. Keep the common sparse_lanczos_svd_config setup in one place, only toggling config.use_mgs2_orthogonalization for the MGS2 variant, and preserve the existing loop_on_state and sparse_lanczos_svd call sites.cpp/tests/sparse/solver/lanczos_svds.cu (2)
88-125: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing coverage for
U/Vt=std::nullopt.
sparse_lanczos_svd_configdocumentsstd::nulloptas a valid way to skip storingU/Vt(persolver_types.hpp), but no test in this file exercises that path. Given RAFT's testing guideline to maintain high coverage of public APIs, consider adding a case that passesstd::nulloptfor one or both outputs.As per coding guidelines, "It's important for RAFT to maintain a high test coverage of the public APIs in order to minimize the potential for downstream projects to encounter unexpected build or runtime behavior."
Also applies to: 205-241, 315-330
🤖 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_svds.cu` around lines 88 - 125, Add a test case in lanczos_svds.cu that exercises sparse_lanczos_svd with std::nullopt for U and/or Vt, since sparse_lanczos_svd_config and the solver API support skipping these outputs. Update the Run helper or add a new helper around sparse_lanczos_svd to call it with nullopt, then verify the singular values still match expectations without materializing the omitted outputs. Use the existing sparse_lanczos_svd, sparse_lanczos_svd_config, and Run symbols to keep the new coverage aligned with the current test structure.Source: Coding guidelines
25-544: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting shared test harness.
The CSR buffer setup → config construction → solve → host copy-back → sync pattern is repeated near-identically across
LanczosSvdsTest::Run,LanczosClusteredSpectrumTest::Run,run_diagonal_hardening_case, andrun_rotated_block_hardening_case. Extracting a common helper (e.g., taking indptr/indices/values + config and returning host S/U/Vt) would reduce duplication and ease future maintenance of this test suite.🤖 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_svds.cu` around lines 25 - 544, The test file repeats the same CSR setup, sparse_lanczos_svd config construction, solve, host copy-back, and stream sync flow in LanczosSvdsTest::Run, LanczosClusteredSpectrumTest::Run, run_diagonal_hardening_case, and run_rotated_block_hardening_case. Extract that sequence into a shared helper that accepts the CSR buffers and sparse_lanczos_svd_config and returns the host S/U/Vt results, then update each of those call sites to use it while keeping only the case-specific assertions and matrix construction in place.
🤖 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_svds.cuh`:
- Around line 124-162: The cuBLAS pointer mode in mgs2_orthogonalize is only
restored on the success path, so an exception from any RAFT_CUBLAS_TRY or
RAFT_CUDA_TRY can leave the shared cublas handle in device mode. Update
mgs2_orthogonalize to use an exception-safe scoped guard, or reuse an existing
RAFT canonical pointer-mode RAII helper if one exists, so the handle is always
returned to CUBLAS_POINTER_MODE_HOST on every exit path. Keep the existing
cublasSetPointerMode setup and ensure the restore happens automatically even if
the loop aborts early.
In `@cpp/include/raft/sparse/solver/lanczos_svds.cuh`:
- Around line 1-16: Move the Lanczos SVDS implementation templates out of the
public header by splitting the existing wrapper in `lanczos_svds.cuh` into the
usual inline/explicit-instantiation pair. Keep only the public wrapper
declarations in the header, move the template definitions from
`detail/lanczos_svds.cuh` into an `-inl.cuh`/`-ext.cuh` layout, and add
`RAFT_COMPILED` / `RAFT_EXPLICIT_INSTANTIATE_ONLY` guards so downstream
translation units do not instantiate the implementation directly. Make sure the
wrapper overloads remain the only public entry points and that the float/double
explicit instantiations used by the `.cu` files are reused.
---
Nitpick comments:
In `@cpp/bench/prims/sparse/svds.cu`:
- Around line 312-371: Consolidate the duplicated benchmark logic in
lanczos_svds_bench and lanczos_mgs2_svds_bench by extracting the shared
run_benchmark flow into one implementation and parameterizing the
orthogonalization choice with a constructor flag or similar field. Keep the
common sparse_lanczos_svd_config setup in one place, only toggling
config.use_mgs2_orthogonalization for the MGS2 variant, and preserve the
existing loop_on_state and sparse_lanczos_svd call sites.
In `@cpp/include/raft/sparse/solver/detail/lanczos_svds.cuh`:
- Around line 251-254: The breakdown threshold in lanczos_svds.cuh is hardcoded
in the Lanczos SVD setup, so it is not scaled to ValueTypeT and is too small for
float. Update the epsilon initialization near the op.rows()/op.cols() setup to
derive the threshold from machine precision for the current ValueTypeT (for
example via std::numeric_limits<ValueTypeT>::epsilon() with an appropriate
scale) so the breakdown check in the Lanczos routine behaves consistently for
both float and double.
- Around line 40-50: The `vector_norm` helper in `lanczos_svds.cuh` is adding a
redundant blocking `resource::sync_stream(handle, stream)` after
`raft::linalg::detail::cublasnrm2`, which already completes before `result` is
read in the default host pointer mode. Remove that extra sync from `vector_norm`
and keep the rest of the Lanczos flow (`normalize_or_randomize` call sites)
unchanged unless there is a documented need for a non-default cuBLAS mode; if
so, add a brief comment near `vector_norm` explaining the requirement.
In `@cpp/tests/sparse/solver/lanczos_svds.cu`:
- Around line 88-125: Add a test case in lanczos_svds.cu that exercises
sparse_lanczos_svd with std::nullopt for U and/or Vt, since
sparse_lanczos_svd_config and the solver API support skipping these outputs.
Update the Run helper or add a new helper around sparse_lanczos_svd to call it
with nullopt, then verify the singular values still match expectations without
materializing the omitted outputs. Use the existing sparse_lanczos_svd,
sparse_lanczos_svd_config, and Run symbols to keep the new coverage aligned with
the current test structure.
- Around line 25-544: The test file repeats the same CSR setup,
sparse_lanczos_svd config construction, solve, host copy-back, and stream sync
flow in LanczosSvdsTest::Run, LanczosClusteredSpectrumTest::Run,
run_diagonal_hardening_case, and run_rotated_block_hardening_case. Extract that
sequence into a shared helper that accepts the CSR buffers and
sparse_lanczos_svd_config and returns the host S/U/Vt results, then update each
of those call sites to use it while keeping only the case-specific assertions
and matrix construction in place.
In `@python/pylibraft/pylibraft/tests/test_sparse.py`:
- Around line 228-405: The new Lanczos tests are implemented as methods on
TestSvds, but the test guideline prefers standalone test_foo_bar() functions.
Move these cases out of the class and make them module-level tests, preserving
the same assertions and parameters in functions like
test_lanczos_diagonal_spectrum, test_lanczos_return_singular_vectors_modes,
test_lanczos_rotated_clustered_spectrum,
test_lanczos_rank_deficient_and_edge_shapes,
test_lanczos_non_convergence_raises, and test_invalid_solver.
🪄 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: bf8109ed-b790-4078-9385-89d9febf71c1
📒 Files selected for processing (21)
cpp/CMakeLists.txtcpp/bench/prims/CMakeLists.txtcpp/bench/prims/sparse/svds.cucpp/include/raft/sparse/solver/detail/lanczos_svds.cuhcpp/include/raft/sparse/solver/detail/randomized_svds.cuhcpp/include/raft/sparse/solver/detail/svds_optional.hppcpp/include/raft/sparse/solver/lanczos_svds.cuhcpp/include/raft/sparse/solver/randomized_svds.cuhcpp/include/raft/sparse/solver/solver_types.hppcpp/include/raft/sparse/solver/svds_config.hppcpp/include/raft_runtime/solver/lanczos_svds.hppcpp/include/raft_runtime/solver/randomized_svds.hppcpp/src/raft_runtime/solver/lanczos_svds.cuhcpp/src/raft_runtime/solver/lanczos_svds_double.cucpp/src/raft_runtime/solver/lanczos_svds_float.cucpp/tests/CMakeLists.txtcpp/tests/sparse/solver/lanczos_svds.cudocs/source/cpp_api/sparse_solver.rstpython/pylibraft/benchmarks/sparse_svds.pypython/pylibraft/pylibraft/sparse/linalg/svds.pyxpython/pylibraft/pylibraft/tests/test_sparse.py
💤 Files with no reviewable changes (1)
- cpp/include/raft/sparse/solver/svds_config.hpp
| template <typename ValueTypeT> | ||
| void mgs2_orthogonalize(raft::resources const& handle, | ||
| ValueTypeT* target, | ||
| ValueTypeT const* basis, | ||
| int n_rows, | ||
| int n_valid, | ||
| ValueTypeT* coeffs) | ||
| { | ||
| common::nvtx::range<common::nvtx::domain::raft> scope("lanczos_svds::mgs2_orthogonalize"); | ||
| if (n_valid <= 0) { return; } | ||
|
|
||
| auto cublas_handle = resource::get_cublas_handle(handle); | ||
| auto stream = resource::get_cuda_stream(handle); | ||
|
|
||
| RAFT_CUBLAS_TRY(cublasSetPointerMode(cublas_handle, CUBLAS_POINTER_MODE_DEVICE)); | ||
| for (int pass = 0; pass < 2; ++pass) { | ||
| for (int j = 0; j < n_valid; ++j) { | ||
| auto const* q_j = basis + static_cast<std::size_t>(j) * n_rows; | ||
| auto* coeff = coeffs + j; | ||
| { | ||
| common::nvtx::range<common::nvtx::domain::raft> dot_scope("lanczos_svds::mgs2_dot"); | ||
| RAFT_CUBLAS_TRY( | ||
| raft::linalg::detail::cublasdot(cublas_handle, n_rows, q_j, 1, target, 1, coeff, stream)); | ||
| } | ||
| { | ||
| common::nvtx::range<common::nvtx::domain::raft> negate_scope("lanczos_svds::mgs2_negate"); | ||
| negate_scalar_kernel<<<1, 1, 0, stream>>>(coeff); | ||
| RAFT_CUDA_TRY(cudaPeekAtLastError()); | ||
| } | ||
| { | ||
| common::nvtx::range<common::nvtx::domain::raft> subtract_scope( | ||
| "lanczos_svds::mgs2_subtract"); | ||
| RAFT_CUBLAS_TRY(raft::linalg::detail::cublasaxpy( | ||
| cublas_handle, n_rows, coeff, q_j, 1, target, 1, stream)); | ||
| } | ||
| } | ||
| } | ||
| RAFT_CUBLAS_TRY(cublasSetPointerMode(cublas_handle, CUBLAS_POINTER_MODE_HOST)); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Restore cuBLAS pointer mode via RAII, not only on the happy path.
If any RAFT_CUBLAS_TRY/RAFT_CUDA_TRY call inside the loop throws, cublasSetPointerMode(..., CUBLAS_POINTER_MODE_HOST) on Line 161 never executes, leaving the shared raft::resources cuBLAS handle stuck in device-pointer mode. Every other cuBLAS call in this file (and elsewhere sharing the same handle) assumes host pointer mode for scalar args, so a later failure here would silently corrupt subsequent computations rather than just aborting cleanly.
As per coding guidelines, "Ensure device memory, streams, and events are cleaned up on all paths; avoid GPU memory leaks, CUDA stream/event leaks, and missing RAII/exception-safe cleanup."
🔒 Proposed RAII guard for cuBLAS pointer mode
+struct cublas_pointer_mode_guard {
+ cublasHandle_t handle_;
+ cublasPointerMode_t prev_mode_;
+ cublas_pointer_mode_guard(cublasHandle_t handle, cublasPointerMode_t mode) : handle_(handle)
+ {
+ RAFT_CUBLAS_TRY(cublasGetPointerMode(handle_, &prev_mode_));
+ RAFT_CUBLAS_TRY(cublasSetPointerMode(handle_, mode));
+ }
+ ~cublas_pointer_mode_guard() { cublasSetPointerMode(handle_, prev_mode_); }
+};
+
template <typename ValueTypeT>
void mgs2_orthogonalize(raft::resources const& handle,
ValueTypeT* target,
ValueTypeT const* basis,
int n_rows,
int n_valid,
ValueTypeT* coeffs)
{
common::nvtx::range<common::nvtx::domain::raft> scope("lanczos_svds::mgs2_orthogonalize");
if (n_valid <= 0) { return; }
auto cublas_handle = resource::get_cublas_handle(handle);
auto stream = resource::get_cuda_stream(handle);
- RAFT_CUBLAS_TRY(cublasSetPointerMode(cublas_handle, CUBLAS_POINTER_MODE_DEVICE));
+ cublas_pointer_mode_guard pointer_mode_guard(cublas_handle, CUBLAS_POINTER_MODE_DEVICE);
for (int pass = 0; pass < 2; ++pass) {
...
}
- RAFT_CUBLAS_TRY(cublasSetPointerMode(cublas_handle, CUBLAS_POINTER_MODE_HOST));
}Please verify whether RAFT already has a canonical scoped pointer-mode guard elsewhere that should be reused instead:
#!/bin/bash
rg -n -C3 'cublasSetPointerMode|CUBLAS_POINTER_MODE' cpp/include cpp/src🤖 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_svds.cuh` around lines 124 -
162, The cuBLAS pointer mode in mgs2_orthogonalize is only restored on the
success path, so an exception from any RAFT_CUBLAS_TRY or RAFT_CUDA_TRY can
leave the shared cublas handle in device mode. Update mgs2_orthogonalize to use
an exception-safe scoped guard, or reuse an existing RAFT canonical pointer-mode
RAII helper if one exists, so the handle is always returned to
CUBLAS_POINTER_MODE_HOST on every exit path. Keep the existing
cublasSetPointerMode setup and ensure the restore happens automatically even if
the loop aborts early.
Source: Coding guidelines
| /* | ||
| * SPDX-FileCopyrightText: Copyright (c) 2026, NVIDIA CORPORATION. | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| #pragma once | ||
|
|
||
| #include <raft/core/device_csr_matrix.hpp> | ||
| #include <raft/core/device_mdspan.hpp> | ||
| #include <raft/core/resources.hpp> | ||
| #include <raft/sparse/solver/detail/csr_linear_operator.cuh> | ||
| #include <raft/sparse/solver/detail/lanczos_svds.cuh> | ||
| #include <raft/sparse/solver/detail/svds_optional.hpp> | ||
|
|
||
| #include <optional> | ||
|
|
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether sibling solver headers (randomized_svds, eigensolver lanczos) follow the split-header pattern,
# to confirm whether this PR's lanczos_svds.cuh is a new deviation or matches existing convention.
fd -e cuh -e hpp . cpp/include/raft/sparse/solver cpp/include/raft/sparse/solver/detail
rg -n "RAFT_EXPLICIT_INSTANTIATE_ONLY|RAFT_COMPILED|extern template" cpp/include/raft/sparse/solver/randomized_svds.cuh cpp/include/raft/sparse/solver/detail/randomized_svds.cuh 2>/dev/nullRepository: NVIDIA/raft
Length of output: 1595
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the public and detail headers for Lanczos SVDS and nearby solver headers.
for f in \
cpp/include/raft/sparse/solver/lanczos_svds.cuh \
cpp/include/raft/sparse/solver/lanczos.cuh \
cpp/include/raft/sparse/solver/randomized_svds.cuh \
cpp/include/raft/sparse/solver/detail/lanczos_svds.cuh \
cpp/include/raft/sparse/solver/detail/lanczos.cuh \
cpp/include/raft/sparse/solver/detail/randomized_svds.cuh
do
echo "===== $f ====="
wc -l "$f"
done
echo
echo "===== public header excerpts ====="
sed -n '1,220p' cpp/include/raft/sparse/solver/lanczos_svds.cuh
echo
sed -n '1,220p' cpp/include/raft/sparse/solver/lanczos.cuh
echo
sed -n '1,220p' cpp/include/raft/sparse/solver/randomized_svds.cuh
echo
echo "===== detail header excerpts ====="
sed -n '1,260p' cpp/include/raft/sparse/solver/detail/lanczos_svds.cuh
echo
sed -n '1,260p' cpp/include/raft/sparse/solver/detail/lanczos.cuh
echo
sed -n '1,260p' cpp/include/raft/sparse/solver/detail/randomized_svds.cuh
echo
echo "===== explicit instantiation files ====="
fd -a -e cu -e cuh -e cpp 'lanczos_svds|randomized_svds|lanczos' cpp/src cpp/include | sort
rg -n "explicit instantiation|extern template|RAFT_EXPLICIT_INSTANTIATE_ONLY|RAFT_COMPILED|template class|template void|template struct" cpp/src cpp/include/raft/sparse/solver -g '*lanczos*' -g '*randomized_svds*'Repository: NVIDIA/raft
Length of output: 47320
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the files that were referenced by the initial results to identify how this area is organized.
fd -a -e cu -e cuh -e cpp 'lanczos_svds|randomized_svds|lanczos' cpp/src cpp/include | sort
rg -n "RAFT_EXPLICIT_INSTANTIATE_ONLY|RAFT_COMPILED|extern template|template class|template void|template struct" \
cpp/src cpp/include/raft/sparse/solver \
-g '*lanczos*' -g '*randomized_svds*'Repository: NVIDIA/raft
Length of output: 1355
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant files and search for split-header patterns in this area.
printf "\n-- file list --\n"
git ls-files \
'cpp/include/raft/sparse/solver/*' \
'cpp/include/raft/sparse/solver/detail/*' \
'cpp/src/raft_runtime/solver/*' | \
rg 'lanczos|randomized_svds|svds'
printf "\n-- public headers --\n"
for f in \
cpp/include/raft/sparse/solver/lanczos_svds.cuh \
cpp/include/raft/sparse/solver/randomized_svds.cuh \
cpp/include/raft/sparse/solver/lanczos.cuh
do
echo "### $f"
sed -n '1,220p' "$f"
echo
done
printf "\n-- detail headers --\n"
for f in \
cpp/include/raft/sparse/solver/detail/lanczos_svds.cuh \
cpp/include/raft/sparse/solver/detail/randomized_svds.cuh \
cpp/include/raft/sparse/solver/detail/lanczos.cuh
do
echo "### $f"
sed -n '1,260p' "$f"
echo
done
printf "\n-- explicit instantiation files --\n"
for f in \
cpp/src/raft_runtime/solver/lanczos_svds_float.cu \
cpp/src/raft_runtime/solver/lanczos_svds_double.cu \
cpp/src/raft_runtime/solver/randomized_svds_float.cu \
cpp/src/raft_runtime/solver/randomized_svds_double.cu
do
if [ -f "$f" ]; then
echo "### $f"
sed -n '1,220p' "$f"
echo
fi
done
printf "\n-- search for RAFT_COMPILED / RAFT_EXPLICIT_INSTANTIATE_ONLY / extern template --\n"
rg -n "RAFT_COMPILED|RAFT_EXPLICIT_INSTANTIATE_ONLY|extern template" \
cpp/include/raft/sparse/solver cpp/src/raft_runtime/solverRepository: NVIDIA/raft
Length of output: 48062
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the runtime wrapper headers and any nearby instantiation plumbing.
for f in \
cpp/src/raft_runtime/solver/lanczos_svds.cuh \
cpp/src/raft_runtime/solver/randomized_svds.cuh \
cpp/src/raft_runtime/solver/lanczos_solver.cuh
do
echo "===== $f ====="
wc -l "$f"
sed -n '1,240p' "$f"
echo
done
echo "===== search for FUNC_DEF / explicit instantiation plumbing ====="
rg -n "FUNC_DEF|RAFT_EXPLICIT_INSTANTIATE_ONLY|RAFT_COMPILED|extern template|template class|template void" cpp/src/raft_runtime/solver cpp/include/raft/sparse/solverRepository: NVIDIA/raft
Length of output: 7292
Split Lanczos SVDS templates out of the public header. cpp/include/raft/sparse/solver/lanczos_svds.cuh still includes detail/lanczos_svds.cuh directly, so downstream TUs will instantiate the implementation even though the runtime .cu files only instantiate the wrapper overloads. Add the usual -inl.cuh/-ext.cuh split with RAFT_COMPILED / RAFT_EXPLICIT_INSTANTIATE_ONLY guards so the prebuilt float/double instantiations can be reused.
🤖 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/lanczos_svds.cuh` around lines 1 - 16, Move
the Lanczos SVDS implementation templates out of the public header by splitting
the existing wrapper in `lanczos_svds.cuh` into the usual
inline/explicit-instantiation pair. Keep only the public wrapper declarations in
the header, move the template definitions from `detail/lanczos_svds.cuh` into an
`-inl.cuh`/`-ext.cuh` layout, and add `RAFT_COMPILED` /
`RAFT_EXPLICIT_INSTANTIATE_ONLY` guards so downstream translation units do not
instantiate the implementation directly. Make sure the wrapper overloads remain
the only public entry points and that the float/double explicit instantiations
used by the `.cu` files are reused.
Source: Path instructions
5 participants
As discussed previously with @cjnolet I'm also adding my Lanczos SVD solver for sparse CSR matrices.
This is the more precise sparse SVD path next to the existing randomized solver. The solver repeatedly applies
A @ vandA.T @ uto build Krylov bases, computes the SVD of the small bidiagonal problem, uses the resulting Ritz vectors to identify converged singular triplets, locks those vectors, and restarts on the remaining unconverged part. It also uses full reorthogonalization and a finalA @ Vrefinement step to improve singular values and left singular vectors.Compared with randomized SVD, this is aimed at quality: clustered spectra, slow singular-value decay, near-rank-deficient inputs, and PCA workloads where ARPACK-like accuracy matters.
Ran the #2999 -style row sweep on the same singlecell dataset, k=50, n_oversamples=10, n_power_iters=2, best-of-3 GPU timings. GPU: RTX PRO 6000 Blackwell.
Using the 2999 CPU baselines for context: