perf(benchmarks): use dual ridge for Gifford2022, Hebart2023_fmri, Papale2025 - #2474
Open
KartikP wants to merge 1 commit into
Open
perf(benchmarks): use dual ridge for Gifford2022, Hebart2023_fmri, Papale2025#2474KartikP wants to merge 1 commit into
KartikP wants to merge 1 commit into
Conversation
…pale2025 #2383 added DualRidgeRegression / DualRidgeCVRegression and switched Allen2022, but three benchmark families were left on the primal path. They are the three worst OOM offenders in the suite. From brainscore_resource_usage: path benchmark n OOM peak dual Li2026 427 20 ( 5%) 165.5 GB dual Zerbe2026 268 76 (28%) 87.2 GB primal Papale2025 159 103 (65%) 249.0 GB primal Gifford2022 48 29 (60%) 246.9 GB primal Hebart2023 159 63 (40%) 217.8 GB The primal path materialises an (n_features, n_targets) coefficient matrix. For Papale2025 that is ~22,000 stimuli against 100k+ features; #2383 measured the equivalent matrix for Allen2022 at 62 GB, reduced to ~1 MB by the dual form. Note the dual form is not a cure on its own -- Zerbe2026 has been dual since it was introduced and still OOMs at 28% -- so this reduces the rate rather than eliminating it. Tolerances are deliberately left at their current abs=0.001 rather than pre-loosened. Allen2022 needed only its IT assertions relaxed, from 0.005 to 0.01, and guessing here would hide the real drift. Plugin-test CI is the measurement: whatever fails reports the actual per-benchmark shift, and the tolerance is then set from that evidence rather than from a guess. Implementation equivalence is already pinned by TestDualRidgeCV in metrics/regression_correlation/test.py (alpha to rel=1e-9, predictions to abs=1e-6). Independently reproduced at larger scale before making this change: at n=100..400 and p=1,000..20,000, dual and primal agree to ~1e-12 relative and select an identical alpha in every case. So any benchmark-level drift is data-dependent alpha selection near a grid tie, not an implementation error.
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.
Follow-up to #2383, which added
DualRidgeRegression/DualRidgeCVRegressionand switched Allen2022 but left three benchmark families on the primal path. Closes out #2364, which I've closed as superseded.Three one-line changes:
load_metric(f'{metric_type}_split')→load_metric(f'dual_{metric_type}_split').Why these three
They are the three worst OOM offenders in the suite. From
brainscore_resource_usage:The primal path materialises an
(n_features, n_targets)coefficient matrix. Papale2025 is ~22,000 stimuli against 100k+ features; #2383 measured the equivalent matrix for Allen2022 at 62 GB, reduced to ~1 MB by the dual form.Not a cure on its own, though. Zerbe2026 has been dual since it was introduced (#2394, 2026-06-06) and still OOMs at 28%, so this reduces the rate rather than eliminating it. I'd expect these three to land somewhere in the 5–28% band rather than at zero.
Tolerance check — the part worth reading
All 8
-ridgecvassertions currently sit atabs=0.001, tighter than Allen2022's original 0.005, which then needed0.01for IT:I have deliberately not pre-loosened any of them. Guessing a tolerance would hide the very thing we want to measure. Plugin-test CI is the measurement: whatever fails reports the actual per-benchmark drift, and each tolerance then gets set from that number rather than from a guess. Expect some of these to fail on the first run — that is the intended outcome, not a problem with the PR.
Implementation equivalence is separately established
So that a CI failure can be read as alpha selection on real data, not the dual form is wrong:
TestDualRidgeCV::test_matches_sklearn(from #2383) already pins alpha torel=1e-9and predictions toabs=1e-6, parametrized over 3 seeds × 3 noise levels.20 passedon this branch.I reproduced it independently at larger scale before making the change, closer to the Brain-Score regime:
Agreement to ~1e-12 relative with an identical alpha in every case. So any benchmark-level drift is data-dependent alpha selection landing on a different grid point near a tie — which is exactly what Allen2022's IT case looks like — not a correctness problem.