Native row-major PCA by aamijar · Pull Request #3036 · NVIDIA/raft

aamijar · 2026-05-26T00:52:52Z

Currently PCA only supports col-major which is not ideal for consumers passing in row-major datasets.
This PR exposes row-major APIs for PCA and calls the corresponding row-major raft primitives based on templated row or col layout.

This PR is backward compatible with cuml and cuvs.

divyegala · 2026-05-28T16:34:02Z

+  auto components_copy_view = raft::make_device_matrix_view<math_t, idx_t, LayoutPolicy>(
+    components_copy.data(), n_components, n_cols);


Why explicitly here instead of inline in the diff before this PR?

divyegala · 2026-05-28T16:34:24Z

+  auto components_copy_view = raft::make_device_matrix_view<math_t, idx_t, LayoutPolicy>(
+    components_copy.data(), n_components, n_cols);


Same as above

divyegala · 2026-05-28T16:44:39Z

+  void to_row_major(const T* col_major_src, T* row_major_dst, int n_rows, int n_cols)
+  {
+    std::vector<T> host_col(n_rows * n_cols);
+    std::vector<T> host_row(n_rows * n_cols);
+    raft::update_host(host_col.data(), col_major_src, n_rows * n_cols, stream);
+    RAFT_CUDA_TRY(cudaStreamSynchronize(stream));
+    for (int i = 0; i < n_rows; ++i) {
+      for (int j = 0; j < n_cols; ++j) {
+        host_row[i * n_cols + j] = host_col[j * n_rows + i];
+      }
+    }
+    raft::update_device(row_major_dst, host_row.data(), n_rows * n_cols, stream);
+  }


Why is this needed? You're generating 1-dimensional random data. It can be interpreted as row or col major.

cjnolet · 2026-06-23T16:23:04Z

+  raft::stats::cov<input_row_major>(
    handle, cov.data(), input.data_handle(), mu.data_handle(), n_cols, n_rows, true, true, stream);

+  // The eigendecomposition of the (symmetric) covariance matrix naturally produces a


But since covar matrix is symmetric, it shouldn't matter if it's column or row layout, right? Why do we need to transpose at all?

Co-authored-by: Divye Gala <divyegala@gmail.com>

aamijar · 2026-06-29T15:16:13Z

Hi, all the comments should be addressed now in the recent batch of commits.

divyegala · 2026-07-02T20:47:54Z

@coderabbitai review

coderabbitai · 2026-07-02T20:48:00Z

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

coderabbitai · 2026-07-02T20:55:48Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 44a63768-4959-412e-8ebd-41c91236aa2b

📥 Commits

Reviewing files that changed from the base of the PR and between 4965cf6 and 63a27ba.

📒 Files selected for processing (8)

cpp/include/raft/linalg/detail/pca.cuh
cpp/include/raft/linalg/detail/tsvd.cuh
cpp/include/raft/linalg/pca.cuh
cpp/include/raft/linalg/strided_reduction.cuh
cpp/include/raft/stats/detail/mean.cuh
cpp/include/raft/stats/mean.cuh
cpp/tests/linalg/pca.cu
cpp/tests/stats/mean.cu

✅ Files skipped from review due to trivial changes (4)

cpp/include/raft/stats/detail/mean.cuh
cpp/include/raft/linalg/strided_reduction.cuh
cpp/include/raft/stats/mean.cuh
cpp/tests/stats/mean.cu

🚧 Files skipped from review as they are similar to previous changes (4)

cpp/include/raft/linalg/pca.cuh
cpp/tests/linalg/pca.cu
cpp/include/raft/linalg/detail/tsvd.cuh
cpp/include/raft/linalg/detail/pca.cuh

📝 Walkthrough

Summary by CodeRabbit

New Features
- PCA now supports both row-major and column-major matrix layouts for fit, transform, inverse transform, and fit-transform.
- Layout awareness is propagated through the PCA/tSVD pipeline so computation and reconstruction match the selected storage layout.
Tests
- Updated PCA tests to exercise advanced scenarios for both row-major and column-major layouts.
- Added/strengthened checks that inverse transform results accurately reconstruct the original input.

Walkthrough

PCA and tSVD functions are generalized with a LayoutPolicy template parameter across internal helpers, public APIs, and tests. The test suite now exercises both row-major and col-major layouts, and several headers also receive SPDX attribution updates.

Changes

PCA/tSVD Layout Generalization

Layer / File(s)	Summary
tSVD core layout-aware helpers `cpp/include/raft/linalg/detail/tsvd.cuh`	`cal_eig`, `sign_flip_components`, `tsvd_transform`, and `tsvd_inverse_transform` are templated on `LayoutPolicy` with layout checks, layout-dependent view construction and indexing, and updated GEMM wiring.
PCA detail helpers built on tSVD layout support `cpp/include/raft/linalg/detail/pca.cuh`	`trunc_comp_exp_vars`, `pca_fit`, `pca_transform`, `pca_inverse_transform`, and `pca_fit_transform` are templated on `LayoutPolicy`, derive `input_row_major` for stats instantiations, and forward layout-aware matrix views into tSVD helpers.
Public PCA API and doc updates `cpp/include/raft/linalg/pca.cuh`	Public wrappers `pca_fit`, `pca_fit_transform`, `pca_inverse_transform`, and `pca_transform` add a `LayoutPolicy` template parameter with updated matrix view types and Doxygen comments.
PCA test coverage for row-major and col-major layouts `cpp/tests/linalg/pca.cu`	`advancedTest` becomes templated on `LayoutPolicy`, allocates `data2` and `data2_back` locally, builds layout-parameterized views, validates inverse reconstruction, and runs for both `raft::row_major` and `raft::col_major`.
SPDX header updates `cpp/include/raft/linalg/detail/pca.cuh`, `cpp/include/raft/linalg/detail/tsvd.cuh`, `cpp/include/raft/linalg/strided_reduction.cuh`, `cpp/include/raft/stats/detail/mean.cuh`, `cpp/include/raft/stats/mean.cuh`, `cpp/tests/linalg/pca.cu`, `cpp/tests/stats/mean.cu`	SPDX copyright header lines are updated in the PCA, tSVD, stats, and test headers.

Estimated code review effort: 4 (Complex) | ~60 minutes

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main change: native row-major PCA support.
Description check	✅ Passed	The description matches the changeset by explaining row-major PCA support and backward compatibility.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

coderabbitai

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

cpp/tests/linalg/pca.cu (1)
135-198: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift

HIGH: advancedTest round-trip does not validate PCA correctness for row-major.

With n_components == params.n_col2 (full rank) and prms.whiten = false, transform followed by inverse reduces to Z·V = X_c·VᵀV = X_c for any orthonormal V. The data2 ≈ data2_back assertion therefore passes regardless of whether the row-major path produces the correct eigenvectors, the correct component ordering, or the correct sign_flip_components result. The only reference-based checks (explained_vars/components/trans_data) run through basicTest, which is col-major only.

Consider adding row-major coverage that (a) truncates (n_components < n_cols) and (b) validates against a reference (e.g. compare eigenvalues/components against the col-major result on the same data). The whitening path (whiten = true, exercising binary_div_skip_zero/binary_mult_skip_zero with LayoutPolicy views) is also untested for both layouts.
🤖 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/linalg/pca.cu` around lines 135 - 198, `advancedTest` only checks
an inverse-transform round trip, which can succeed even if the row-major PCA
path is wrong. Update `advancedTest` to use a non-full-rank case with
`n_components < n_cols` and add a reference comparison for the row-major
`pca_fit_transform`/`pca_inverse_transform` results against the col-major path
on the same input, including `components`, `explained_vars`, and `trans_view`
ordering/sign handling. Also add coverage for the `prms.whiten = true` path so
the `LayoutPolicy`-dependent branches in
`binary_div_skip_zero`/`binary_mult_skip_zero` are exercised.
Source: 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.

Outside diff comments:
In `@cpp/tests/linalg/pca.cu`:
- Around line 135-198: `advancedTest` only checks an inverse-transform round
trip, which can succeed even if the row-major PCA path is wrong. Update
`advancedTest` to use a non-full-rank case with `n_components < n_cols` and add
a reference comparison for the row-major
`pca_fit_transform`/`pca_inverse_transform` results against the col-major path
on the same input, including `components`, `explained_vars`, and `trans_view`
ordering/sign handling. Also add coverage for the `prms.whiten = true` path so
the `LayoutPolicy`-dependent branches in
`binary_div_skip_zero`/`binary_mult_skip_zero` are exercised.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: d53b3a91-b4d4-40ca-a12f-a8e17df5734e

📥 Commits

Reviewing files that changed from the base of the PR and between 1240331 and 4965cf6.

📒 Files selected for processing (4)

cpp/include/raft/linalg/detail/pca.cuh
cpp/include/raft/linalg/detail/tsvd.cuh
cpp/include/raft/linalg/pca.cuh
cpp/tests/linalg/pca.cu

lowener

LGTM

pca-row-major

bd0a12b

aamijar requested a review from a team as a code owner May 26, 2026 00:52

github-project-automation Bot added this to Unstructured Data Processing May 26, 2026

aamijar self-assigned this May 26, 2026

aamijar added non-breaking Non-breaking change improvement Improvement / enhancement to an existing function labels May 26, 2026

aamijar moved this to In Progress in Unstructured Data Processing May 26, 2026

aamijar mentioned this pull request May 26, 2026

Native row-major PCA NVIDIA/cuvs#2125

Open

divyegala reviewed May 28, 2026

View reviewed changes

cjnolet reviewed Jun 23, 2026

View reviewed changes

Update cpp/include/raft/linalg/detail/pca.cuh

2d9e0e2

Co-authored-by: Divye Gala <divyegala@gmail.com>

aamijar requested a review from a team as a code owner June 27, 2026 17:17

aamijar and others added 6 commits June 27, 2026 13:17

Merge branch 'main' into pca-row-major

774ae45

LayoutPolicy cal_eig

48405e3

refactor inline view

4c29774

clean up

f0b7527

clean up tsvd

b88550d

clean up

4965cf6

josephine-wolf-oberholtzer added this to Unstructured Data Processing Jul 1, 2026

josephine-wolf-oberholtzer moved this to In Progress in Unstructured Data Processing Jul 1, 2026

divyegala reviewed Jul 2, 2026

View reviewed changes

Comment thread cpp/include/raft/linalg/detail/pca.cuh

coderabbitai Bot reviewed Jul 2, 2026

View reviewed changes

aamijar and others added 2 commits July 2, 2026 18:18

Merge branch 'main' into pca-row-major

265781f

Merge branch 'main' into pca-row-major

c7b52c7

divyegala approved these changes Jul 6, 2026

View reviewed changes

lowener approved these changes Jul 7, 2026

View reviewed changes

fix style

63a27ba

		auto components_copy_view = raft::make_device_matrix_view<math_t, idx_t, LayoutPolicy>(
		components_copy.data(), n_components, n_cols);

Uh oh!

Conversation

aamijar commented May 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

divyegala May 28, 2026

Choose a reason for hiding this comment

Uh oh!

divyegala May 28, 2026

Choose a reason for hiding this comment

Uh oh!

divyegala May 28, 2026

Choose a reason for hiding this comment

Uh oh!

cjnolet Jun 23, 2026

Choose a reason for hiding this comment

Uh oh!

aamijar commented Jun 29, 2026

Uh oh!

Uh oh!

divyegala commented Jul 2, 2026

Uh oh!

coderabbitai Bot commented Jul 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot commented Jul 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Walkthrough

Changes

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

lowener left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

aamijar commented May 26, 2026 •

edited

Loading

coderabbitai Bot commented Jul 2, 2026 •

edited

Loading

coderabbitai Bot commented Jul 2, 2026 •

edited

Loading