Skip to content

fix(svd): keep nonneg/L1 constraints on deflation factors after the first - #83

Merged
zdebruine merged 1 commit into
mainfrom
fix/deflation-constrained-orthogonalization
Oct 19, 2026
Merged

zdebruine merged 1 commit into
mainfrom
fix/deflation-constrained-orthogonalization

Conversation

@zdebruine

Copy link
Copy Markdown
Owner

fix(svd): keep nonneg/L1 constraints on deflation factors after the first

Problem

svd()/pca() with method = "deflation" re-orthogonalizes each new factor against the previous ones (two-pass Gram-Schmidt). For factors with projection constraints this undoes the projection: with nonneg = c(TRUE, FALSE) and k = 3, components 2–3 came back with 35–180 negative loadings (of 50–200 features), and L1 zeros were lost on every factor after the first. The existing test acknowledged this as a "known limitation".

method = "auto" also routed constrained problems with k >= 8 to "krylov", whose block solve-then-project step can return redundant (non-deflated) or all-zero components when k > 1.

Changes

  • Deflation (CPU deflation.hpp, GPU sparse + dense deflation_gpu.cuh): skip post-factor Gram-Schmidt on any side with nonneg, L1, or upper_bound. Constrained factors are returned as fitted (non-negative/sparse, not mutually orthogonal), as in other constrained/sparse PCA methods. Unconstrained sides are still re-orthogonalized.
  • Warm start: constrained factors k > 1 start from a non-negative random vector instead of the orthogonalized power-step warm start, which can point into the negative orthant and collapse to zero after projection.
  • R/svd.R: method = "auto" selects "deflation" whenever nonneg/L1/upper_bound is set; explicitly requesting "krylov" with these constraints and k > 1 emits a warning.
  • Tests (test_svd.R): non-negativity asserted on all deflation factors (replacing the "known limitation" note); new tests for mixed-sign data, L1 zeros on later factors, auto-method choice, and the Krylov warning. Existing Krylov constraint tests wrap the call in suppressWarnings().
  • NEWS.md entry.

Verification (CPU, single thread, OpenBLAS; simulated data)

Non-negative PCA (pca(t(X), k = 3, nonneg = c(TRUE, FALSE), method = "deflation")) vs CRAN nsprcomp 0.5.1.2 (nneg = TRUE, 5 restarts):

data (cells × genes) negative loadings before → after cumulative adjusted variance vs nsprcomp PC1 cosine
3,000 × 50 78 → 0 1.009× 0.997
12,000 × 200 355 → 0 1.008× 1.000
12,000 × 200 (planted sparse programs) 158 → 0 1.003× 1.000
100,000 × 100 194 → 0 1.004× 1.000
  • L1 (L1 = c(0.5 | 2 | 5, 0)): exact zeros preserved on every factor (13–98 per column).
  • Unconstrained deflation SVD unchanged: max |U'U − I| 1.4e-7, max |V'V − I| 7.9e-8, singular values match base::svd to 4e-4 (float32).
  • Full test suite: 939 tests, 0 failures, 0 errors (485 skipped: GPU / optional packages).

Not covered here (follow-ups)

  • GPU changes mirror the CPU logic but were not compiled or run (no CUDA device available).
  • Krylov constrained path (krylov_constrained_svd) still needs a proper deflation scheme for k > 1; this PR only steers auto away from it and warns.
  • configure and cleanup are tracked without the executable bit, so R CMD INSTALL from a fresh clone fails until they are chmod +x'd (git update-index --chmod=+x configure cleanup).

Generated by Claude Code

…irst

The deflation solver re-orthogonalized every factor against the previous
ones with two-pass Gram-Schmidt. For factors with projection constraints
(nonneg, L1, upper_bound) this re-introduced negative entries and removed
L1 zeros on all components after the first, so `pca(..., nonneg = TRUE)`
with k > 1 returned mixed-sign loadings.

- Skip post-factor Gram-Schmidt on any side with projection constraints
  (CPU and both GPU deflation paths); unconstrained SVD is unchanged.
- Start constrained factors k > 1 from a non-negative random vector rather
  than the orthogonalized power-step warm start, which could collapse to
  zero after projection.
- method = "auto": choose deflation whenever nonneg/L1/upper_bound is set;
  warn when krylov is requested with these constraints and k > 1 (its block
  solve-then-project can return redundant or all-zero components).
- Tests: assert non-negativity on all deflation factors, L1 zeros on later
  factors, auto-method choice, and the krylov warning.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014zFkHdSUAE1RSwGtTqz9Fi

Copy link
Copy Markdown
Owner Author

CI note: test-coverage failure is not from this PR.

The job fails in r-lib/actions/setup-r-dependencies before any test runs:

Could not solve package dependencies:
* deps::.: Can't install dependency SeuratData
* SeuratData: Can't find package called SeuratData.

SeuratData is listed in Suggests but is not on CRAN and DESCRIPTION has no Remotes: entry, so pak cannot build the lockfile. This PR does not touch DESCRIPTION or the workflows, and main (df69ddd) fails the same jobs. The R-CMD-check jobs use the same dependency step and will likely fail the same way.

No fix exists on main yet. Proposed one-line patch (separate PR, so this one stays scoped):

 Suggests:
     ...
     SeuratData,
     ...
+Remotes:
+    satijalab/seurat-data

Alternatively, drop SeuratData from Suggests: it is only used by vignettes/gpu-acceleration.Rmd, a guarded block in vignettes/regularization.Rmd, and scripts in tools/. Locally the full testthat suite passes on this branch (939 tests, 0 failures, 485 skipped for GPU/optional packages). Re-running the job once.


Generated by Claude Code

@zdebruine
zdebruine merged commit 5cf3c0f into main Oct 19, 2026
0 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant