feat(svd): implement svd in rust without using openblas as a dependency#7518
feat(svd): implement svd in rust without using openblas as a dependency#7518SarahNasser576 wants to merge 38 commits into
Conversation
…hat does not depend on OpenBLAS
…r values σᵢ = √λᵢ for the top k eigenvalues
…eigenvectors to build the U matrix.
…fill these zero columns with orthonormal vectors that complete the basis of R^m.
…xn identity matrix as a row-major vector
… to a symmetric matrix S. Also created the apply_givens_right() function, which accumulates a Givens rotation into the matrix V from the right
…ct of the specified number of rows and number of columns
… least one row and at least one column, from an assert statement to an if statement.
…f the matrix matches the product of the specified number of rows and number of columns, from an assert statement to an if statement.
…ns null or infinite entries, from an assert statement to an if statement.
… messages can be displayed if needed
… my input validation error messages
…nput matrices will be displayed if the length of the input matrix is 0, even if the number of specified rows and columns is not 0.
…empty, even if the specified number of rows and columns is not 0
github-actions
Bot
added
A-index
|
ACTION NEEDED The PR title and description are used as the merge commit message. Please update your PR title and description to match the specification. For details on the error please inspect the "PR Title Check" action. |
SarahNasser576
changed the title
|
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:
📝 WalkthroughWalkthroughThis PR adds a self-contained Rust SVD implementation with ChangesRust SVD implementation
Formatting and tooling
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested labels: 🚥 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tall pylance (1)
1-174: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStray diff artifact committed as a source file.
This file's name and content indicate it is a leftover/accidentally added file containing a raw
git diffoutput rather than actual notebook source. It should be removed from the PR before merge.🤖 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 `@tall` pylance around lines 1 - 174, This notebook appears to be a stray diff artifact rather than a real source change, so remove the accidentally committed raw git diff content from report.ipynb. Verify the intended notebook file is not meant to be added here, and if the benchmark notebook change is unnecessary, delete the file from the PR; otherwise replace it with a valid notebook source using the existing notebook structure and identifiers like make_plot and the benchmark cells.
🤖 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 `@pre-commit-config.yaml`:
- Around line 2-11: Normalize the list indentation in the pre-commit
configuration so the repository entries and hook items use consistent YAML
spacing. Update the top-level sequence formatting in pre-commit-config.yaml
around the repo and hooks entries so the dashes align with standard YAML style,
and ensure the config remains valid for yaml linting in CI. Use the existing
pre-commit hook blocks as the target for the indentation cleanup.
In `@rust/lance-linalg/src/svd.rs`:
- Around line 138-183: The gram_schmidt function is cloning each previously
processed column inside the inner loops, which adds avoidable allocations and
extra O(ncols² · dim) work. Update gram_schmidt to avoid cols[j].clone() by
restructuring the borrows, such as using split_at_mut or an immutable
prefix/slice for the already-processed columns while mutating cols[i]. Keep the
same orthogonalization and normalization behavior, but remove the per-iteration
Vec copies in both the main projection loop and the zero-vector fallback path.
- Around line 200-218: Update svd to stop printing validation failures and
returning empty vectors; instead make the function return a Result<(Vec<f64>,
Vec<f64>, Vec<f64>)> and use the crate’s typed invalid-input error for bad
caller data. Consolidate the three input checks into one validation path in svd,
remove the duplicated predicates and the stdout println! calls, and return an
error that includes m, n, and a.len() so callers can distinguish invalid input
from a real empty result.
- Around line 191-200: The public svd API currently uses plain comments, so it
has no rustdoc and is missing the required example and parameter/output
documentation. Replace the existing comment block above svd with proper rustdoc
comments, clearly describing the row-major input layout, the meaning of m and n,
and the ordering/shape of U, sigma, and V^T, and include a minimal usage example
for svd in the docs.
---
Outside diff comments:
In `@tall` pylance:
- Around line 1-174: This notebook appears to be a stray diff artifact rather
than a real source change, so remove the accidentally committed raw git diff
content from report.ipynb. Verify the intended notebook file is not meant to be
added here, and if the benchmark notebook change is unnecessary, delete the file
from the PR; otherwise replace it with a valid notebook source using the
existing notebook structure and identifiers like make_plot and the benchmark
cells.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 952001da-107f-4b16-8cdd-2d95cb785baf
📒 Files selected for processing (14)
benchmarks/full_report/report.ipynbbenchmarks/sift/Results.ipynbci/check_breaking_changes.pyci/setup_version.pymemtest/tests/integration_test.rsnotebooks/quickstart.ipynbnotebooks/youtube_transcript_search.ipynbpre-commit-config.yamlrust/lance-linalg/src/svd.rsrust/lance/benches/mem_wal/vector/hnsw/disk_ann_compare.pyskills/lance-user-guide/scripts/python_end_to_end.pytall pylancetest_data/v0.30.0_pre_created_at/datagen.pytest_debug.py
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
rust/lance-linalg/src/svd.rs-198-204 (1)
198-204: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFix the
svddoctest return value
svd(...)returns a tuple, so the?in the example prevents the doctest from compiling. Drop it so the example matches the function signature.🤖 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 `@rust/lance-linalg/src/svd.rs` around lines 198 - 204, Update the `svd` doctest example to remove the `?` from the `svd(...)` call, since the example should destructure the returned tuple directly and match the function signature.Source: Coding guidelines
🤖 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.
Other comments:
In `@rust/lance-linalg/src/svd.rs`:
- Around line 198-204: Update the `svd` doctest example to remove the `?` from
the `svd(...)` call, since the example should destructure the returned tuple
directly and match the function signature.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: 2842132d-7906-4713-8bc1-fce89747f849
📒 Files selected for processing (1)
rust/lance-linalg/src/svd.rs
There was a problem hiding this comment.
Actionable comments posted: 1
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
rust/lance-linalg/src/svd.rs (2)
75-95: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftPerform full Jacobi sweeps before accepting convergence.
Line 75 eliminates only one pivot per iteration, so
MAX_JACOBI_SWEEPScaps rotations—not sweeps. Dense matrices can return non-diagonalizedS, producing invalid eigenvectors and an incorrect SVD. Perform up toMAX_JACOBI_SWEEPS * n * (n - 1) / 2rotations or implement cyclic sweeps, and add a reconstruction/orthogonality regression test for a dense matrix requiring more than the current cap.🤖 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 `@rust/lance-linalg/src/svd.rs` around lines 75 - 95, Update the Jacobi iteration around the pivot search in the SVD eigen-decomposition so MAX_JACOBI_SWEEPS represents complete sweeps rather than individual rotations; allow up to MAX_JACOBI_SWEEPS * n * (n - 1) / 2 pivot rotations, or implement equivalent cyclic sweeps. Preserve convergence checks while ensuring dense matrices are fully diagonalized, and add a regression test covering reconstruction and eigenvector orthogonality for a dense matrix requiring more than the current rotation cap.
265-272: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not zero valid singular values using an absolute cutoff.
A finite input with a singular value in
(0, EPS * 10]loses that component, soU * Σ * Vᵀno longer reconstructsA. Only use the placeholder path for zero singular values (after clamping negative numerical eigenvalue noise), and add a small-scale diagonal-matrix regression test.Proposed fix
- let mut sigma: Vec<f64> = order[..k] + let sigma: Vec<f64> = order[..k] ... - if sigma[index] > EPS * 10.0 { + if sigma[index] > 0.0 { u_cols.push(av.iter().map(|&x| x / sigma[index]).collect()); } else { - sigma[index] = 0.0; u_cols.push(vec![0.0; m]); }🤖 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 `@rust/lance-linalg/src/svd.rs` around lines 265 - 272, Update the singular-value handling in the SVD construction around sigma[index] so every positive singular value is normalized, rather than discarded by the absolute EPS * 10.0 cutoff; reserve the placeholder zero-column path for values that are zero after clamping negative numerical eigenvalue noise. Add a regression test using a small-scale diagonal matrix with a singular value in (0, EPS * 10] and verify that U * Σ * Vᵀ reconstructs the input.
🟡 Other comments (1)
rust/lance-linalg/src/svd.rs-223-227 (1)
223-227: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winReport the offending non-finite matrix entry.
Find the first non-finite value and include its index, value,
m,n, anda.len()in the error; also change “null” to “non-finite,” sincef64has no null state.As per coding guidelines, include full error context including variable names, values, sizes, and types.
🤖 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 `@rust/lance-linalg/src/svd.rs` around lines 223 - 227, Update the input validation in the SVD implementation to find the first non-finite entry rather than only checking all values, and return an invalid-input error containing its index and value plus m, n, and a.len(). Change the validation wording from “null” to “non-finite,” and include the relevant variable names, values, sizes, and types in the error context.Source: Coding guidelines
🤖 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 `@rust/lance-linalg/src/svd.rs`:
- Around line 218-221: Update the SVD input validation around the data-length
check to compute m*n with checked_mul and return a descriptive
Error::invalid_input containing m, n, and a.len() when it overflows. Apply the
same checked multiplication before any downstream use of m*m or n*n, ensuring
all overflowing dimensions are rejected at the API boundary before allocation or
indexing.
---
Outside diff comments:
In `@rust/lance-linalg/src/svd.rs`:
- Around line 75-95: Update the Jacobi iteration around the pivot search in the
SVD eigen-decomposition so MAX_JACOBI_SWEEPS represents complete sweeps rather
than individual rotations; allow up to MAX_JACOBI_SWEEPS * n * (n - 1) / 2 pivot
rotations, or implement equivalent cyclic sweeps. Preserve convergence checks
while ensuring dense matrices are fully diagonalized, and add a regression test
covering reconstruction and eigenvector orthogonality for a dense matrix
requiring more than the current rotation cap.
- Around line 265-272: Update the singular-value handling in the SVD
construction around sigma[index] so every positive singular value is normalized,
rather than discarded by the absolute EPS * 10.0 cutoff; reserve the placeholder
zero-column path for values that are zero after clamping negative numerical
eigenvalue noise. Add a regression test using a small-scale diagonal matrix with
a singular value in (0, EPS * 10] and verify that U * Σ * Vᵀ reconstructs the
input.
---
Other comments:
In `@rust/lance-linalg/src/svd.rs`:
- Around line 223-227: Update the input validation in the SVD implementation to
find the first non-finite entry rather than only checking all values, and return
an invalid-input error containing its index and value plus m, n, and a.len().
Change the validation wording from “null” to “non-finite,” and include the
relevant variable names, values, sizes, and types in the error context.
🪄 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: QUIET
Plan: Pro Plus
Run ID: 5a964796-7c57-4801-b78e-8dd6167de3cc
📒 Files selected for processing (1)
rust/lance-linalg/src/svd.rs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rust/lance-linalg/src/svd.rs (1)
75-95: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftMake Jacobi convergence scale-aware and actually sweep the matrix.
Each loop performs only one rotation, so the 100-iteration cap is not 100 sweeps and leaves larger matrices substantially non-diagonalized. The absolute
EPS * 1e4cutoff also immediately rejects valid small-scale inputs: forA = [1e-5, 1e-5](m=1,n=2),AᵀAhas off-diagonals of1e-10, so no rotation occurs and the returned singular value is wrong.Use a relative tolerance based on the matrix norm, perform full
(p, q)sweeps (or dimension-scaled iterations), and return an error if convergence is not reached. Add reconstruction tests for scaled rank-deficient and matrices requiring more than 100 rotations.🤖 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 `@rust/lance-linalg/src/svd.rs` around lines 75 - 95, Update the Jacobi eigensolver loop around the off-diagonal maximum search to perform complete sweeps over all eligible (p, q) pairs, rather than applying only one rotation per outer iteration; scale convergence against the matrix norm so small inputs such as AᵀA with 1e-10 off-diagonals are processed correctly. Track whether convergence is reached within the dimension-scaled iteration limit, return an error when it is not, and add reconstruction tests covering scaled rank-deficient inputs and matrices needing more than 100 rotations.
🤖 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.
Outside diff comments:
In `@rust/lance-linalg/src/svd.rs`:
- Around line 75-95: Update the Jacobi eigensolver loop around the off-diagonal
maximum search to perform complete sweeps over all eligible (p, q) pairs, rather
than applying only one rotation per outer iteration; scale convergence against
the matrix norm so small inputs such as AᵀA with 1e-10 off-diagonals are
processed correctly. Track whether convergence is reached within the
dimension-scaled iteration limit, return an error when it is not, and add
reconstruction tests covering scaled rank-deficient inputs and matrices needing
more than 100 rotations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: 3f70bb7f-0bc8-4d2c-a9f3-4eb7db776dbc
📒 Files selected for processing (1)
rust/lance-linalg/src/svd.rs
2 participants
What does this PR do?
This PR implements the classical eigendecomposition singular value decomposition (SVD) algorithm in Rust without using OpenBLAS as a dependency.
Why was this PR needed?
I needed to implement an SVD algorithm without using OpenBLAS as a dependency because when computing SVD for the Optimized Product Quantization (OPQ) indexing process, many problems occur in building and using OpenBLAS as a dependency across different systems.
What are the relevant issue numbers?
Closes #984
Does this PR meet the acceptance criteria?
All integration and unit tests are passing.
Integration tests
Unit tests