funNystrom++ - #132
Closed
mmelnich wants to merge 14 commits into
Closed
Conversation
Timing: - NystromEVD: bool timing flag + times[11] (alloc, syrf, matvec, gram, potrf, trsm, svd, post_svd, error_est, rest, total); += accumulation across adaptive iterations; skip error_est block entirely when error_est_p==0 && tol==0 (the fixed-rank FunNystromPP path) - LanczosFA: bool timing flag + times[5] (matvec, run_lanczos, apply_f, rest, total); _t_matvec_us accumulated inside run_lanczos per A-call, reset at top of both run_lanczos and call(); workspace_per_thread formula written as d*d+4*d-1 to match comment - FunNystromPP benchmark: nystrom_evd.timing=true, lfa_pp.timing=true; per-phase printf breakdown; 32 CSV columns (up from 16) Bug fixes: - power_error_est: inner loop variable shadowed outer iter; m=1 column- scaling bug from && i!=0 guard; replaced with explicit double loop - power_error_est: 1.0/0.0 literals -> (T)1/(T)0 for float safety; same fix applied throughout NystromEVD::call() - NystromEVD::call(): int i -> int64_t i in post-SVD eigval loop - nystrom_pc_data: auto out_state captured int return of call() instead of advanced RNGState; fixed to NystromAlg.call(); return state; - Benchmark printf: nt[9] (nystrom_rest) was dropped from console output - LFAOp: scalar_t hardcoded as double; use typename SLO_t::scalar_t - call_all_algs: state param is never modified; changed to const& Safety: - Deleted copy ctor/assignment on NystromEVD, LanczosFA (+ explicit default ctor), Hutchinson (+ explicit default ctor), FunNystromPP to prevent double-free on accidental copy Test: - ASSERT_NEAR(err, target, 10*target) -> ASSERT_LT(err, 10*target)
mmelnich
added a commit
that referenced
this pull request
May 14, 2026
…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.
Contributor
Author
|
Reworking this PR in #138 |
mmelnich
added a commit
that referenced
this pull request
May 25, 2026
…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]).
mmelnich
added a commit
that referenced
this pull request
Aug 6, 2026
Native Windows (MSVC) support, mirroring the recipe RandBLAS PR #179 validated: Visual Studio 2022, Intel oneMKL from vcpkg with **ILP64 + sequential** linking (RandBLAS's MKL sparse backend requires 64-bit `MKL_INT`), and BLAS++/LAPACK++ built from the BallisticLA branches that carry the two one-line MSVC fixes still open upstream (blaspp PR #132, lapackpp PR #87). An audit of the whole tree found the **library headers need zero code changes** for MSVC — every blocker was at the edges. Four commits: 1. **RandBLAS submodule bump** to current main. The old pin (May 2026) predates RandBLAS's Windows support, so this is a prerequisite; it also brings in three sketching-performance PRs (#173, #174, #176). The full local suite — RandLAPACK's 337 tests plus RandBLAS's 449 — passes against the new pin. 2. **MSVC portability, sources + CMake**: - Tests: `__PRETTY_FUNCTION__` (GCC/Clang-only) → the `__RANDBLAS_PRETTY_FUNCTION__` macro RandBLAS's `comparison.hh` now provides (36 sites; every affected file already includes that header). - Benchmarks: the repository's only two C variable-length arrays (`gejsv` integer workspaces) become heap allocations; a pre-existing workspace leak in `find_test_mat_spectrum` is freed along the way. - CMake: warning flags and benchmark `-g` become compiler-conditional (MSVC gets `/W4`); the header-only target exports `/utf-8` to MSVC consumers (several headers carry non-ASCII characters in string literals); `SANITIZE_ADDRESS` learns the MSVC spelling; new `CMake/rl_runtime_dlls.cmake` stages dependency DLLs beside test executables (`gtest_discover_tests` runs them at build time); the `lapackpp_DIR` recorded in the installed package config is normalized with `TO_CMAKE_PATH` so Windows backslash paths don't arrive as escape sequences downstream; the template-heavy test translation units get `/bigobj`. 3. **`core-windows` workflow**: windows-2022 runner, cached dependency stack via a composite action, Ninja build, full ctest (standard ABRIK exclusion). The job is named `build-windows`, deliberately distinct from the required check `build`, so it can prove itself before being made required. (RandBLAS's own tests ran inside this job in the first CI rounds; item 7 turns them off.) 4. **`install.ps1`**: thin companion to `install.sh` — reuses the CI dependency script (so the recipe exists exactly once), supports `-MklRoot` for oneAPI installs instead of vcpkg, and mirrors the `..\RandNLA-project` layout. INSTALL.md gains a native-Windows section. **OpenMP is off on native Windows for now**: `rl_rpchol.hh` uses an OpenMP `collapse(2)` clause that MSVC only accepts under `/openmp:llvm`, while RandBLAS's build system pins MSVC OpenMP to `/openmp:experimental`. All OpenMP use is guarded, so serial builds are fully functional; reconciling the two runtimes is a RandBLAS-side follow-up. Also catalogued but deliberately not fixed here (they never touch ILP64 BLAS calls): the `long`-typed timing fields overflow at ~35.8 minutes per timer on Windows (LLP64), and `rl_memory_tracker.hh`'s byte arithmetic overflows at 2 GiB working sets — diagnostics-only issues, queued for post-1.0 cleanup. Verified on Linux after all changes: clean build, 785/786 tests (the one failure is a documented machine-local GPU issue predating this work). The first Windows execution is this PR's own CI run. --- **Added after the first CI round:** 5. **Fix from CI iteration 1**: `rl_hqrrp.hh` and three BQRRP benchmarks kept dead `#ifdef LAPACK_FORTRAN_STRLEN_END` blocks *inside* LAPACK macro invocations — undefined behavior that GCC/Clang tolerate and MSVC rejects (C2121). Pure deletion; the lapackpp variadic macros append the string-length arguments themselves. 6. **`install-windows` CI job**, filling the slot reserved in the installer workflow: runs `install.ps1` end-to-end plus an idempotent re-run, sharing the cached dependency stack with `core-windows` (new `-DependencyRoot` installer option). 7. **Core-CI speedups** (measured before: core-linux 10–12 min, core-windows 12–16): BLAS++/LAPACK++/Random123 installs are now cached in core-linux/core-macos; builds use all runner cores instead of `-j2`; the RandBLAS submodule's ~450 tests no longer build and run inside every RandLAPACK job (`-DBUILD_TESTS=OFF` — the pinned commit is already tested by RandBLAS's own CI; the installer lanes keep them for user parity); and the Debug/ASan build+suite is a parallel `build-asan` job instead of running sequentially inside `build`. The required-check job name `build` is unchanged. 8. **Installer scripts moved to `install/`** (`install/install.sh`, `install/install.ps1`), with a two-line wrapper kept at the root so `bash install.sh` keeps working — the CI installer lanes call the wrapper on purpose so it stays under test. 9. **`docs/CI.md`**: a map of the CI setup — what each of the eight jobs validates, which checks are required, what is deliberately red (the core-macos canary) or disabled (submodule tests, Windows OpenMP), cache keys and how to bust them, and per-OS local-reproduction commands. 10. **RandBLAS bump to `/openmp:llvm`** (post-RandBLAS#184) and **an OpenMP matrix leg in core-windows**: with RandBLAS selecting MSVC's LLVM OpenMP runtime, RandLAPACK's OpenMP code (64-bit loop indices, the `collapse(2)` in rl_rpchol) compiles under MSVC for the first time. The workflow becomes serial + openmp, mirroring RandBLAS's own Windows matrix; the "OpenMP disabled on MSVC" caveat retires from INSTALL.md. 11. **install.ps1 sanity-check fix**: the clone check tested a path that never existed (`RandLAPACK/RandLAPACK.hh`; the umbrella header sits at the repo root), so the install-windows job had never gotten past it.
mmelnich
added a commit
that referenced
this pull request
Aug 12, 2026
…upstream, pinned
Two findings from running every backend path against a real oneMKL install.
1. The architecture fix was necessary but NOT sufficient. With a correct x64
toolchain and our own dgemm_/dgesv_ link check passing, BLAS++ still
reported "BLAS library not found". Its try_compile log gives the real
reason:
ninja: error: 'C:/Program', needed by 'cmTC_x.exe', missing
BLAS++ feeds library paths into that probe unquoted, so a path containing
a space splits at the space. Intel installs oneMKL to
C:\Program Files (x86)\Intel\oneAPI\ by default, so every *discovered*
oneMKL hits this, as does any custom backend under Program Files. It was
invisible to CI and to earlier local runs because the downloaded oneMKL
lands in a space-free directory under the dependency root -- the one
layout that avoids it. Proven by copying the identical libraries to a
space-free path, after which BLAS++ finds them immediately.
Worked around by staging backend import libraries into a space-free
directory when, and only when, a path contains a space. Import libraries
only name their DLL, which is still resolved at run time from the
backend's bin directory, so this is safe. The underlying quoting bug is
BLAS++'s and is worth reporting upstream.
2. BLAS++ and LAPACK++ were cloned from BallisticLA forks carrying two
one-line MSVC fixes. Both merged upstream on 2026-08-06 (blaspp #132,
lapackpp #87), so the forks are obsolete; both now come from
icl-utk-edu. Verified the pinned commits carry the fixes.
They were also pinned to *branch names*, i.e. a moving tip, inside a cache
keyed on this script -- so a cache hit could restore a different revision
than a cache miss builds. That is the same defect already fixed here for
Random123. Clone-Head is now Clone-Pinned and takes a tag or a commit SHA,
so every dependency is pinned to an immutable ref: blaspp 3057185,
lapackpp 40b9d0d, GoogleTest v1.17.0, Random123 v1.14.0. Commits rather
than tags for the first two only because the latest release of each,
v2025.05.28, predates the merges. Cache keys renamed off the fork branches
and bumped, so no fork-built artifact can be restored.
Verified on Windows against a real oneAPI install at the default (spaced)
location: discovery path 749/749 tests, and a full build from the upstream
pins 749/749, with both clones confirmed to originate from icl-utk-edu.
Also corrects two unverified doc claims: the quick start recommended a VS
edition and winget invocation that had never been run (missing --wait, which
returns while the installer is still going), and INSTALL_SCRIPT.md suggested
cmake@3.27 two paragraphs after stating the floor is 3.21.
mmelnich
added a commit
that referenced
this pull request
Aug 12, 2026
…ller fetches The installer never supplies a compiler, CMake or Ninja -- those are the user's to provide, same as on Linux and macOS. It does fetch the BLAS/LAPACK backend and the build-time dependencies, and until now the versions were only discoverable by reading setup.ps1. Adds a table naming each component, its exact pinned version, its upstream source and how it is verified, plus two points that were implicit: - oneMKL is downloaded ONLY when no existing oneAPI is discovered. When one is found the installer uses the user's version, so the documented version applies to the no-oneMKL case only. - BLAS++ and LAPACK++ are pinned to commits rather than to their latest release, v2025.05.28, because that release predates the two one-line MSVC fixes this build needs (blaspp #132, lapackpp #87, merged upstream 2026-08-06). They move to a release tag once one includes them. Everything else is a stable, released version. Sections renumbered to keep the new one in place; internal cross-references updated.
mmelnich
added a commit
that referenced
this pull request
Aug 12, 2026
…upstream, pinned
Two findings from running every backend path against a real oneMKL install.
1. The architecture fix was necessary but NOT sufficient. With a correct x64
toolchain and our own dgemm_/dgesv_ link check passing, BLAS++ still
reported "BLAS library not found". Its try_compile log gives the real
reason:
ninja: error: 'C:/Program', needed by 'cmTC_x.exe', missing
BLAS++ feeds library paths into that probe unquoted, so a path containing
a space splits at the space. Intel installs oneMKL to
C:\Program Files (x86)\Intel\oneAPI\ by default, so every *discovered*
oneMKL hits this, as does any custom backend under Program Files. It was
invisible to CI and to earlier local runs because the downloaded oneMKL
lands in a space-free directory under the dependency root -- the one
layout that avoids it. Proven by copying the identical libraries to a
space-free path, after which BLAS++ finds them immediately.
Worked around by staging backend import libraries into a space-free
directory when, and only when, a path contains a space. Import libraries
only name their DLL, which is still resolved at run time from the
backend's bin directory, so this is safe. The underlying quoting bug is
BLAS++'s and is worth reporting upstream.
2. BLAS++ and LAPACK++ were cloned from BallisticLA forks carrying two
one-line MSVC fixes. Both merged upstream on 2026-08-06 (blaspp #132,
lapackpp #87), so the forks are obsolete; both now come from
icl-utk-edu. Verified the pinned commits carry the fixes.
They were also pinned to *branch names*, i.e. a moving tip, inside a cache
keyed on this script -- so a cache hit could restore a different revision
than a cache miss builds. That is the same defect already fixed here for
Random123. Clone-Head is now Clone-Pinned and takes a tag or a commit SHA,
so every dependency is pinned to an immutable ref: blaspp 3057185,
lapackpp 40b9d0d, GoogleTest v1.17.0, Random123 v1.14.0. Commits rather
than tags for the first two only because the latest release of each,
v2025.05.28, predates the merges. Cache keys renamed off the fork branches
and bumped, so no fork-built artifact can be restored.
Verified on Windows against a real oneAPI install at the default (spaced)
location: discovery path 749/749 tests, and a full build from the upstream
pins 749/749, with both clones confirmed to originate from icl-utk-edu.
Also corrects two unverified doc claims: the quick start recommended a VS
edition and winget invocation that had never been run (missing --wait, which
returns while the installer is still going), and INSTALL_SCRIPT.md suggested
cmake@3.27 two paragraphs after stating the floor is 3.21.
mmelnich
added a commit
that referenced
this pull request
Aug 12, 2026
…ller fetches The installer never supplies a compiler, CMake or Ninja -- those are the user's to provide, same as on Linux and macOS. It does fetch the BLAS/LAPACK backend and the build-time dependencies, and until now the versions were only discoverable by reading setup.ps1. Adds a table naming each component, its exact pinned version, its upstream source and how it is verified, plus two points that were implicit: - oneMKL is downloaded ONLY when no existing oneAPI is discovered. When one is found the installer uses the user's version, so the documented version applies to the no-oneMKL case only. - BLAS++ and LAPACK++ are pinned to commits rather than to their latest release, v2025.05.28, because that release predates the two one-line MSVC fixes this build needs (blaspp #132, lapackpp #87, merged upstream 2026-08-06). They move to a release tag once one includes them. Everything else is a stable, released version. Sections renumbered to keep the new one in place; internal cross-references updated.
mmelnich
added a commit
that referenced
this pull request
Aug 12, 2026
…l pinned Everything the installer can fetch is now the newest stable release of that component, pinned to an exact version: oneMKL 2025.2.0.627 -> 2026.1.0.226 (was four releases behind) GoogleTest v1.17.0 -> v1.18.0 OpenBLAS 0.3.34 already newest Random123 v1.14.0 already newest BLAS++ and LAPACK++ stay pinned to commits rather than to v2025.05.28: that release predates the two MSVC fixes this build needs (blaspp #132, lapackpp #87). They move to a tag once one carries them; tracked in #158. The oneMKL bump renames the runtime DLLs from mkl_*.2.dll to mkl_*.3.dll. Nothing hardcodes those names -- staging globs *.dll -- so the change is transparent, but it would have silently staged nothing had anything hardcoded them. The package layout is otherwise unchanged (build/native/win-x64, build/native/include, runtimes/win-x64/native). Cache keys bumped for oneMKL and both GoogleTest variants, so no artifact built from the old versions can be restored. Verified on Windows, both provisioning paths: - discovery against an installed 2026.1.0 oneAPI: link check OK - forced NuGet download of 2026.1.0.226: both SHA256s verified, unpacked, "Found BLAS library", mkl_core.3.dll / mkl_sequential.3.dll staged - GoogleTest v1.18.0 is a major bump touching every test: 749/749 pass
mmelnich
added a commit
that referenced
this pull request
Aug 12, 2026
References like "blaspp #132" and "lapackpp #87" were written bare. GitHub resolves a bare #NNN against the repository it is rendered in, so these linked to RandLAPACK #132 and #87 -- unrelated pull requests (funNystrom++ and a benchmarking DNM) -- rather than to the upstream fixes they name. A reader following them lands somewhere plausible and wrong, which is worse than a dead link. Now written as icl-utk-edu/blaspp#132 and icl-utk-edu/lapackpp#87, which GitHub renders as cross-repository links. Same for the other upstream references in these files.
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
1 participant
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