perf(rust): compiled MAP demeaning kernel - rayon across columns, 1.4-2.8x on FE-absorption workloads#603
Merged
Merged
Conversation
…umns, GIL released Optional Rust acceleration for FE-absorption demeaning, mirroring the canonical numpy engine exactly (python-canonical policy): - rust/src/demean.rs: per-column independent convergence loops, same sweep order, row-order scatter-add accumulation (np.bincount parity), division by per-group sums, zero-total-weight inert guard, max|x - x_old| < tol with NaN-poisoning semantics. Owned copies first, then Python::detach releases the GIL around the rayon compute; Fortran-order result so per-column views are contiguous. Codes validated against n_groups while copying. Cargo unit tests included. - diff_diff/utils.py: numpy MAP loops extracted into _demean_map_numpy (the reference engine, directly testable) + _demean_map_rust wrapper (explicit dtype coercion, None contract honored when the kernel symbol is absent, deliberate "demean_map" marker errors -> numpy fallback, others re-raised). demean_by_groups dispatches rust-then-numpy; warning contract, snap/LSMR stage, one-way delegation, n_effects unchanged. - diff_diff/_backend.py: demean_map imported INDEPENDENTLY so a stale or mixed-version extension missing only this symbol degrades to the numpy engine without disabling older Rust accelerations. Equivalence (tests/test_rust_backend.py::TestDemeanMapKernel, 16 cases): iteration-count EQUALITY then assert_allclose atol=1e-12 across weighted/unweighted x 2-way/3-way x balanced/unbalanced/contiguous(>100 iters), zero-weight inertness, NaN non-convergence parity, k=1/64, non-convergence flags, forced-fallback + stale-symbol fallback, warning parity, estimator-level ATT/SE + FE-spanned snap-decision parity. Outputs bit-identical on fixtures (asserted at allclose per policy). Sweeps: 960 passed (rust) + 875 passed (DIFF_DIFF_BACKEND=python). Measured on frozen code vs a fresh same-session numpy run (CVs <= 4.1%): firm_churn 48.98s -> 25.84s (1.90x; CV-adjusted lower bound 1.78x, submit gate >= 1.5x PASS), tail_stress 2.79x, geo 2.02x (now ahead of the pyfixest yardstick), scanner 1.59x, survey 1.44x, county 1.36x, guard_small unregressed. Identity vs committed after-baselines 1e-13-1e-16 incl. tail_stress. Memory trade-off disclosed: firm_churn peak RSS 13.4 -> 21.0 GB (kernel input copy + result; variable-chunking is the noted follow-up). Solver-bound county-class finding promoted to an Actionable TODO row. REGISTRY/doc-deps/CHANGELOG updated. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Overall Assessment ✅ Looks good. No unmitigated P0/P1 findings. Executive Summary
Methodology
Code Quality
Performance
Maintainability
Tech Debt
Security
Documentation/Tests
Review note: I did not run the full test suite in this read-only review environment; static inspection included |
igerber
added a commit
that referenced
this pull request
Jul 3, 2026
…ase memory attribution (#605) Adds an internal, opt-in memory lever for the Rust FE-absorption kernel and corrects a wrong attribution shipped with #603: - diff_diff/utils.py: _demean_map_rust can dispatch variables to the kernel in balanced near-equal column blocks (DIFF_DIFF_DEMEAN_CHUNK_COLS env knob, read per-call; invalid/non-positive values fall back silently per the DIFF_DIFF_BACKEND convention). DEFAULT IS OFF (single full-width dispatch - byte-identical behavior to #603). Chunking is exact partitioning: per-column outputs and iteration counts are unchanged by construction (no cross-column arithmetic), locked by exact-equality tests vs single-call dispatch plus numpy-engine parity at atol=1e-12. Balanced bounds (never a small remainder block) because a 2-column tail measured ~+11% wall-clock: it runs nearly serial across the rayon pool. - Corrected memory attribution (docs/performance-plan.md): the rust-backend firm-panel fit's ~21GB peak (vs ~13.4GB numpy-backend) is dominated by the SOLVER phase (faer path holds an owned copy of the ~2.5GB stacked design + SVD workspace), NOT the demean kernel's marshalling as the #603 narrative claimed. Measured: width-16 chunked dispatch cuts the kernel's transients to near-numpy footprint in isolation, yet the end-to-end fit still peaks ~19-21GB (-5-12%) at +2-7% wall-clock - which is why the knob ships opt-in rather than default-on (pre-agreed gate: >=20% RSS reduction AND <=5% slowdown; measured result fails both legs). Measurement note recorded: single-run macOS ru_maxrss is unreliable under ambient memory pressure (compressor deflates resident peaks). - TODO.md: new Actionable/Performance row for rust solve_ols peak memory on wide designs (the actual dominator); solver-bound SunAbraham speed row demoted Actionable -> Parked per the correctness-first decision (QR-reuse removes the SVD's independent near-collinearity detector and perturbs all coefficients; re-open on practitioner demand, gate on fixest/R parity). - tests/test_rust_backend.py: TestDemeanMapKernel +5 (chunked-vs-single exact equality incl. weighted, boundary splits with kernel call-count assertions, cross-chunk non-convergence warning naming, estimator-level ATT/SE parity at chunk=2, default-is-single-dispatch proof) and standalone TestDemeanChunkResolver (env parsing, invalid fallback, runs without the Rust build). All chunk tests delenv-guard against ambient DIFF_DIFF_DEMEAN_CHUNK_COLS exports. - REGISTRY.md note extended (opt-in knob, OFF by default, invariance by construction); doc-deps utils note updated; CHANGELOG Added entry. No Rust changes; no committed baseline changes (default path identical, PR-C-session fe_absorption_rust.json retained). Sweeps: 461 passed (rust) / 371 passed (DIFF_DIFF_BACKEND=python) on the targeted files. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Merged
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
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.
Summary
demean_mapkernel for FE-absorption demeaning (rust/src/demean.rs): the method-of-alternating-projections sweeps run compiled and rayon-parallel across the demeaned variables, with the GIL released (Python::detach) around the compute. No BLAS dependency - works on the plain Windows build.diff_diff.utils._demean_map_numpy(the directly-testable reference), and the kernel reproduces its per-column independent convergence loops, dimension sweep order, row-order scatter-add accumulation (np.bincountparity), per-group-sum division, zero-total-weight inert guard, andmax|x - x_old| < tolstopping rule including NaN-poisoning semantics. Equivalence tests assert iteration-count EQUALITY thenassert_allclose(atol=1e-12); observed outputs are bit-identical on all fixtures (asserted at allclose per policy - never a bit-identity claim).demean_mapis imported independently in_backend.py, so a stale/mixed-version extension missing only the new symbol degrades to the numpy engine without disabling older Rust accelerations; the wrapper honors a documented None-fallback contract (absent symbol, degenerate shapes, or kernel-side validation errors) and re-raises anything else.DIFF_DIFF_BACKEND=pythondisables as usual. Everything downstream (non-convergence warning contract, FE-spanned-regressor snap + LSMR confirmation, one-way delegation,n_effects) is unchanged.baselines/fe_absorption_rust.json): firm_churn (2.4M-row SunAbraham) 48.98s -> 25.84s = 1.90x (CV-adjusted lower bound 1.78x - clears the pre-agreed >= 1.5x submit gate), correlated-FE stress 31.74s -> 11.37s (2.79x), 5M-row geo experiment 0.976s -> 0.484s (2.02x, now ahead of the pyfixest yardstick's 0.623s), scanner TWFE 1.59x, survey BRR 1.44x, county event study 1.36x, small-panel guard unregressed. Estimate identity vs the committed baselines: 1e-13-1e-16 on every scenario including the stress case.Methodology references (required if estimator / math changes)
docs/methodology/REGISTRY.md"Absorbed Fixed Effects with Survey Weights" (new Note covers the Rust kernel contract)Validation
tests/test_rust_backend.py::TestDemeanMapKernel(16 cases: weighted/unweighted x 2-way/3-way x balanced/unbalanced/contiguous >100-iteration regimes, zero-total-weight inertness, NaN-in-variable non-convergence parity, k=1/k=64, non-convergence flag parity, forced-fallback and stale-symbol fallback both proven to run the numpy engine, warning parity, estimator-level ATT/SE + FE-spanned snap-decision parity); cargo unit tests inrust/src/demean.rs. Full targeted sweeps: 960 passed with the Rust build, 875 passed underDIFF_DIFF_BACKEND=python.baselines/fe_absorption_rust.json(multi-run protocol, CVs recorded); findings table indocs/performance-plan.mdgains Rust columns viagen_findings_tables.py; gate evaluation documented in the performance-plan narrative.Security / privacy
Generated with Claude Code