Review — gram-greedy¶
- Sources:
src/placecell_gram_greedy.cpp,src/placecell_gram_greedy.h(namespace placecell::gram_greedy:State,Proposal, the two constants, the declarations),src/placecell_cull_method.h(CullScope,CullExecutor, shared by every method) - Reviewer: Alejandro Fontan
- Last reviewed at:
74acd51(2026-09-26) — 1 of 4 functions read; the file is the culler split out ofsrc/placecell.cpp
Checklist¶
| Function | Source | State | Notes |
|---|---|---|---|
gram_greedy::cull |
placecell_gram_greedy.cpp#L145 | unread | |
gram_greedy::seed |
placecell_gram_greedy.cpp#L23 | question | unchecked LDLT (#3), jitter gap, see 2026-09-26 |
gram_greedy::propose |
placecell_gram_greedy.cpp#L49 | unread | |
gram_greedy::downdate |
placecell_gram_greedy.cpp#L95 | unread | |
gram_greedy::report |
placecell_gram_greedy.cpp#L118 | unread |
CullExecutor::operator() (header-only, src/placecell_cull_method.h) is not listed; note it under the driver if it matters. The functions were renamed on 2026-09-29 (PlaceCell::gram_greedy_<step> → gram_greedy::<step>, cull_gram_greedy → gram_greedy::cull); the dated notes below keep the old names. report is new (split out of the driver).
Notes¶
2026-09-26 — gram_greedy_seed (at 74acd51)¶
Mis the LDLT solve of the jittered alive block against the identity and the result is never inspected: on an indefinite kernel (an unclippedset_kernelmatrix) someM_iicome out non-positive andgram_greedy_proposedrops those views without a word, so the culler goes quiet with an empty report. The only signal isset_kernel's PSD WARN at load time, far from the symptom. Already the proposal of the kernel-contract issue (a check on the pivots here). → #3- The seeded
v_h = K_hh − w_h · k_hAuses the unjitteredK_hhandk_hAagainst the jitteredM, so it differs from the Schur identity1/M_iithatgram_greedy_downdateassigns to views culled in the same call by a gap of order ε. Harmless at 1e-6; the reference entry states it. → reference - The history rows are seeded in count-driven mode too, where tau is infinite and the price test
cannot fail, so their only consumer is
worst_historyin the report: O(|H||A|²) for a diagnostic. Costs nothing today (offline runs start with no history); skip them whentauis infinite if an online count-driven use ever appears. → reference - Two spellings of one operation: the jitter is added inside the copy loop here and to the
diagonal after the copy in
solve_information, and the two constants are separate (gram_greedy_jittervs a literal insolve_information) with a comment promising they are equal. One named constant in the header would make the promise a fact.<cmath>in this file is unused. → #6
2026-09-26 — checklist created (at 74acd51)¶
No findings yet.