FunNystrom++ V2#138
Open
mmelnich wants to merge 14 commits into
Open
Conversation
…entize
Reference-aligned Persson-Kressner port with bit-exact MATLAB↔C++
cross-validation at LAPACK noise floor.
- Phase 5: vendored RandLAPACK::LanczosFA + BlockLanczosFA from
funnystrompp; util::upsize raw-pointer overload, util::symmetrize.
- Phase 6: SASO sketch (RandBLAS::SparseSkOp) + SkOp-aware first matvec
via right_spmm through ExplicitSymLinOp overload.
- Phase 7a: Cholesky-fast / eig(YᵀY) HMT §5.1 path in NystromEVD_v2.
5-rep median speedup vs SVD-pinv fall-back: 2.71×-3.80× across
k∈{500,1000,1500,2000} at n=2000 (3.80× at k=n=2000, exceeds the
~2× claim in funnystrompp PR #132).
- Phase 7a-perf: force_fallback knob + tight t_specrec_ms inner timer.
- Gap port (#1,#2,#3,#5): optional f_zero correction, Phase 2 skip at
k==n, gesdd(G)→syevd(G) in fall-back, SkOp overload on
FunNystromPP_v2::call.
- Componentize: NystromEVD_v2 + workspace moved to drivers/rl_nystrom_evd_v2.hh.
12 cross-validation fixtures pass at tol=1e-11 (10/12 at strict
tol=1e-12; 2 misses are alg_poly.t2 ~1.4e-12 long-standing cancellation
noise). v2 unit tests 4/4. Full RandLAPACK suite 309/309.
Closed
…pace, 11-slot timing Bundle #2 of the v2-polish cycle: * Rename rl_nystrom_evd_v2.hh → rl_nystrom_evd.hh; NystromEVD_v2 free function → NystromEVD; NystromEVD_v2_workspace → NystromEVD_workspace. * Convert workspace from std::vector<T> members to raw T* + int64_t pairs grown via util::upsize and freed in a destructor, matching PR #132 / rl_revd2.hh patterns. Output params now use (T*&, int64_t&) references-to-pointer. * Add 11-slot timing vector (alloc / syrf / matvec / gram / potrf / trsm / svd / post_svd / error_est=0 / rest / total) with per- phase chrono instrumentation; gated on ws.times_enabled. Layout mirrors PR #132's NystromEVD for cross-driver tooling parity. Slot 8 (error_est) is unused in this fixed-k variant; kept at index 8 to preserve the V1 layout. * FunNystromPP_v2: U, lambda, Y_2, fAOmega, Y0 promoted from stack std::vectors to persistent raw-pointer class members; nystrom_ws promoted to a class member so allocations are reused across repeated call() invocations. Destructor frees all five buffers. FunNystromPP_v2's public call signature is unchanged. Verification: * make -j8 + make install -j8 + benchmark rebuild: clean * v2 unit tests: 4/4 PASS (FullRankCapture err_t1 = 2.426e-16) * 12-fixture cross-validation at tol=1e-12: 10/12 pass, same 2 alg_poly.t2 cancellation-regime cells as pre-refactor (rel_Δ = 4.241e-13, 1.399e-12). est at noise floor across all 12 fixtures (rel_Δ ∈ [0.0, 3.835e-15]).
**Silence all RandLAPACK compiler warnings under `-Wall -Wextra`** Resolves all 22 warnings emitted when building the workshop docker image (`ghcr.io/rileyjmurray/randlapack-python-workshop:latest`). **Categories fixed** (-Wunused-but-set-variable, -Wunused-parameter, -Wsign-compare, -Wvolatile, -Wdangling-else): - Removed dead code: `prevnormNR` in `rl_determiter.hh`; unused `[rows_B, cols_B]` structured bindings in `rl_dense_linop.hh` / `rl_composite_linop.hh`. - Removed genuinely vestigial parameters: `lda` from sparse `ABRIK::call` (LDA is meaningless for sparse — `test_ABRIK_general` uses `if constexpr` to dispatch dense vs. sparse), `k` from `test_csr_col_split`, `norm_A` from `test_CQRRPT_orthogonalization`. All callers updated. - For parameters that are unused only in one specialization but required by template-overload symmetry (`density`/`layout` in `make_operator`), used the existing `UNUSED(x)` macro from `RandBLAS/exceptions.hh` with explanatory comments. - Misc: braced `ASSERT_FALSE` in `test_rpchol.cc` for `-Wdangling-else`; rewrote `x += i` on `volatile int` as `x = x + i` to avoid C++20 deprecation in `test_memory_tracker.cc`. Added issue #137 to deal with clang warnings/errors (will involve updating RandBLAS submodule).
… for UW Madison workshop
) Two RandLAPACK sources triggered clang `-Wunused-but-set-variable` and `-Wmisleading-indentation` under `-Wall -Wextra`. GCC 13.3 heuristics do not flag either, which is why they passed through the prior warnings-cleanup PR #136. `RandLAPACK/testing/rl_gen.hh:174`: removed dead `int idx = offset;` and `++idx;` inside `gen_exp_singvals`; the variable was written but never read. `RandLAPACK/drivers/rl_abrik.hh:281, 286, 415, 419`: de-indented the bodies of four `#ifdef RandBLAS_HAS_OpenMP` blocks so they align with the surrounding function-body indent. The previous extra indent visually chained off the preceding single-statement `if(this->timing)`, which is what triggers `-Wmisleading-indentation`. No control-flow change. Verified inside ghcr.io/rileyjmurray/randlapack-python-workshop with clang 20.1.8: 0 warnings under `-Wall -Wextra` (down from 46), 708/708 ctest pass. GCC 13.3 build unchanged.
Two related bugs were present in both the CPU (rl_bqrrp.hh) and GPU (rl_bqrrp_gpu.hh) implementations of BQRRP. They are independent and the fix is in two parts: 1. Termination condition (CPU and GPU) The loop termination used `curr_sz >= n`. For wide A (m < n), curr_sz tops out at min(m, n) = m < n, so this condition never fires on full-rank wide input. The main loop exhausts maxiter iterations and falls through `return 0` without setting `this->rank` or running the termination handler. Result: `rank` is left at its default (0). Fix: use `curr_sz >= std::min(m, n)`. 2. reconcile_and_cleanup wide-tail copy (GPU only) The GPU's pointer-swap dance (Algorithm 7 of the BQRRP paper, used instead of the CPU's in-place column permutation) needs an explicit reconciliation step at termination to bring data home from whichever buffer holds the latest writes. The existing reconciliation: * Walks block-aligned column ranges [0, processed) where processed = (iter + 1) * b_sz_const (or smaller if block_rank != b_sz_const). * Conditionally copies the trailing region past `processed` only when `iter != maxiter - 1` (intended to handle early termination). For wide A at full completion: processed = m < n, so a valid R12 tail exists in [m, n) but the existing condition `iter != maxiter - 1` skips the copy (full completion has iter == maxiter - 1). Additionally, the J pivot vector has no trailing copy at all on the odd-iter path, leaving stale iter-(k-1) values in J[m:n] for wide A. Since iter k's col_swap pulls values from those positions into earlier slots, the final J ends up with duplicates. Fix: replace the `iter != maxiter - 1` gate with `processed < n`, which handles both early termination and wide A at full completion. Add a symmetric J wide-tail copy on the odd-iter path. The same fix applies in both the block_zero early-exit and the normal termination paths. The CPU implementation does not need the second fix because it uses in-place column permutation -- the factorization data ends up directly in caller's A across iterations, so loop fallthrough only loses the `rank` field. This explains why the wide CPU performance results in the BQRRP paper (Fig 3.12) were unaffected by the bug -- timing didn't depend on the rank output. Tests added: * test_bqrrp.cc::BQRRP_wide_aspect (CPU, m=2000, n=5000) * test_bqrrp_gpu.cu::BQRRP_GPU_wide_aspect (GPU, m=1000, n=2000) The upstream test suite previously had no wide-A coverage (all tests were tall m=5000, n=2800 or square 1000 x 1000), which is why this bug went undetected. Verified via an equivalent fix in a standalone CUDA port of BQRRP: residual went from ~5e-1 (with both bugs) to ~1.5e-15 (machine epsilon) on a m=256, n=512 Gaussian input. The reverse-engineered fix in that port matches the patches here. Co-authored-by: Maksim Melnichenko <mmelnichenko@icsi.berkeley.edu>
Route every PR to @mmelnich as code owner. Pairs with forthcoming branch protection on main that requires Code Owner review.
Converted 150+ printf statements to std::cout across library, tests, and benchmarks (not using streams where applicable yet). Issue #75.
…ion (#140) Four coordinated changes preparing RandLAPACK for the bindings repo: install.sh dependency discovery via BLASPP_INSTALL_DIR / LAPACKPP_INSTALL_DIR / RANDOM123_INSTALL_DIR env vars; skip clones and reuse existing installs when set. Also fixes a CUDA flag conflict when external blaspp depends on CUDAToolkit. New RandLAPACK/rl_exceptions.hh defines RandLAPACK::Error plus a single `randlapack_require(cond) << "msg " << x;` macro backed by a RAII StreamThrower whose destructor throws at end of full expression. Registered in the umbrella header and CMakeLists.txt. The macro uses `for(; !(cond); )` rather than `if(cond);else` to short circuit without triggering -Wdangling-else inside unbraced outer ifs. 211 call sites across 19 RandLAPACK source files migrate from randblas_require to randlapack_require with hand readable messages that include the actual parameter values, e.g. `randlapack_require(b_sz >= 1) << "BQRRP::call: b_sz=" << b_sz << " must be positive"`. Polarity flips from "error if BAD" to "require GOOD"; (long long) casts drop since operator<< handles int64_t directly. Driver entry point validation in every driver's call() and constructor, closing the segfault paths that bindings would expose: BQRRP (8 checks), RSVD (5), CQRRT (7), CQRRPT (8), REVD2 (6 across 2 overloads), ABRIK (9 across 3 overloads), krill_full_rpchol (6 new + 1 retained mu_size).
Coordinates RandLAPACK with RandBLAS PRs #158, #169, and #172. Each
change stands alone; the commit history reflects that grouping.
### Submodule bump
`6abdfcf` → `8417f4b` (current `origin/main`), covering #158 (test
infrastructure refactor), #160, #164, #169 (CMake improvements), #170,
#171, and #172.
### Required follow-on changes from #169
**`Random123::Random123` rename.** RandBLAS #169 retired the bare
`Random123` INTERFACE target in favor of the namespaced
`Random123::Random123 IMPORTED GLOBAL`. The `target_link_libraries`
entry in `RandLAPACK/CMakeLists.txt` is updated to match.
**`RandBLAS_DIR` layout.** RandBLAS #169 moved the installed config from
`lib/cmake/RandBLASConfig.cmake` to
`lib/cmake/RandBLAS/RandBLASConfig.cmake`. The fallback in
`RandLAPACKConfig.cmake.in` now points at
`${CMAKE_CURRENT_LIST_DIR}/../RandBLAS` instead of
`${CMAKE_CURRENT_LIST_DIR}/..`.
### Test migration (forced by #158)
RandBLAS #158 promoted the test-only comparison and sparse-data
utilities from `RandBLAS/test/` headers into the library proper under
`RandBLAS/RandBLAS/testing/`. They install with the library; consumers
reach them via the `RandLAPACK::RandLAPACK` transitive include path.
Eleven files updated (10 RandLAPACK tests + 1 extras test):
| Before | After |
|---|---|
| `"../../RandBLAS/test/comparison.hh"` |
`<RandBLAS/testing/comparison.hh>` |
| `"../../RandBLAS/test/test_datastructures/test_spmats/common.hh"` |
`<RandBLAS/testing/sparse_data.hh>` |
| `test::comparison::` | `RandBLAS::testing::` |
| `test::test_datastructures::test_spmats::` | `RandBLAS::testing::` |
`extras/test/CMakeLists.txt` drops the now-obsolete probe that added the
RandBLAS source tree to the include path to resolve a test-only header
that wasn't installed. Post-#158 those headers are part of the library
install and reach consumers through the package's normal include path;
re-pointing the probe at the new source location would re-introduce the
same anti-pattern, so it's deleted.
### Consumer-facing CMake target improvements (ported from #169)
- Build-time-only flags wrapped in `$<BUILD_INTERFACE:...>` so `-Wall
-Wextra` (and `-fsanitize=address` when `SANITIZE_ADDRESS=ON`) apply
in-tree but disappear from the installed export.
- `target_compile_features(RandLAPACK INTERFACE cxx_std_20)` advertises
the language requirement on the exported target.
- `RandLAPACKConfig.cmake` calls `find_dependency(OpenMP COMPONENTS
CXX)` directly when RandLAPACK was built with OpenMP, rather than
relying on RandBLAS' Config to incidentally pull it into scope.
- `EXPORT_LINK_INTERFACE_LIBRARIES` (deprecated since CMake 2.8.12)
removed from `install(EXPORT)`.
### `RandLAPACK_BUILD_TESTS` option
New `option(RandLAPACK_BUILD_TESTS "Build RandLAPACK's test suite" ON)`
gates `add_subdirectory(test)` at the top level. The name is prefixed so
it can coexist with RandBLAS's own `BUILD_TESTS` when RandBLAS is
embedded as a subdirectory; the two options control their respective
test subdirs independently.
`install.sh` is updated to pass `-DBUILD_TESTS=OFF
-DRandLAPACK_BUILD_TESTS=ON` to the RandLAPACK cmake invocation: the
end-user installation path doesn't run RandBLAS's tests anyway (they're
covered by RandBLAS's own CI) and skipping them shaves measurable time
off the install.
|
|
rileyjmurray
requested changes
May 30, 2026
rileyjmurray
left a comment
rileyjmurray
left a comment
Contributor
There was a problem hiding this comment.
I recommend you have Claude pull all changes from the benchmark/ files on this branch and move them to a new PR. Then you can reduce the scope of this PR to actually be about FunNystrom++.
…e_bin helpers to testing utils, add LanczosFA/NystromEVD timing + reorth flag
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
WIP