[WIP] Refactor fitting logic - #64
Open
christianbrodbeck wants to merge 103 commits into
Open
Conversation
`g_group` and `proxg_group_opt` reshaped their input by assigning to `array.shape`, mutating a caller-owned array (FASTA's live coefficient array) and restoring it afterward. This is fragile (a raise between the two assignments leaves the array mis-shaped) and not thread-safe. Use `reshape()` views instead: `g_group` no longer touches its input, and `proxg_group_opt` writes the Cython shrinkage into a reshaped view that shares the input buffer and returns that view. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
`NCRF.cvfunc`/`_get_cvfunc` embedded the per-fold cross-validation scoring loop on the estimator, reaching into `RegressionData` internals and building fold models, while `_crossvalidation.crossvalidate` called back into `model.cvfunc` through an informal duck-typed contract. Move the scoring loop into `_crossvalidation._score_mu`, which takes the estimator and owns the split/fit/score/aggregate flow end-to-end. The worker helpers now receive the estimator directly instead of a bound method, and `NCRF` exposes only `_new_solver` to cross-validation. The `NCRFModel`/`FitHistory` imports are deferred inside `_score_mu` to avoid an import cycle. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
`_model.py` had grown into a 1800-line module spanning nine unrelated responsibilities. Relocate each group into a dedicated module (pure moves, no logic changes): - _typing.py: shared type aliases and the numeric tolerance - _linalg.py: gaussian_basis and the matrix/covariance kernels - _penalties.py: l1/group penalties and their proximal operators - _forward.py: ForwardModel - _data.py: RegressionData and covariate_from_stim - _initialization.py: find_mu, wls, mne_initialization - _solver.py: FitHistory, _evaluate_objective, Solver, find_mu_range _model.py now holds only NCRF, NCRFModel, and NCRFResult. The public package surface is unchanged; __init__ and _ncrf re-export from the new locations, and the test/doc references to moved names are updated. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
`NCRFModel` accepted a `stim_normalization` argument, stored it as `_stim_normalization`, and never read it — dead freight copied into the frozen model on every fit. Remove the constructor parameter, the attribute, and the `_from_solver` pass-through. `RegressionData.stim_normalization` remains as the computed record of the pre-normalization spectral norms (the source of truth for that quantity, preserved across whiten/timeslice). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The Champagne loop selected the per-source gamma update with a three-way branch that mixed conventions: two branches assigned a return value while the `dc == 3` branch mutated `gamma[i]` in place via the Cython kernel. Add `_linalg.compute_gamma(z, x, dc)`, which presents all three paths as a single return-value interface (allocating the output buffer and calling the in-place kernel internally for the fast path). The solver loop is now a single `gamma[i] = compute_gamma(z, x, dc)`. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
voxelwise_explained_variance reused a single `theta` array as a scratch buffer across the whole computation: it was allocated once (as a copy of self.theta) outside the trial loop, then mutated in place inside the inner per-source loop. Because it was never reset between trials, the baseline residual variance for every trial after the first was computed from a theta that still had the last source's block zeroed from the previous trial's inner loop, corrupting the per-source deltas. Replace the mutate-in-place approach with a direct linear computation: zeroing a source's weights only removes that source's contribution to the prediction, so add the contribution back to the full-model residual. This removes the scratch buffer entirely, computes a consistent baseline for every trial, and touches only each source block's columns of the lead field rather than the whole matrix. Introduce ForwardModel.source_block(i) for the orientation-block slice and use it here and in Solver._solve, removing three copies of the i*dc slice arithmetic. The corrected baseline changes the metric's value, so the test expectation is updated. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
NCRFModel conflated three concerns: weight storage, whitened-space prediction/scoring, and reconstruction of the Gabor coefficients into labeled response-function NDVars. The reconstruction concern owned roughly half the class's state (basis, stim dims/names, baseline/scaling, timing) even though none of it is touched by the scoring methods. Extract that concern into a TRFDesign dataclass in the new _reconstruction module. It holds the stimulus/basis metadata and provides reconstruct() / reconstruct_scaled(), which the model's h / h_scaled now delegate to. NCRFModel stores a single _design object and exposes tstart/tstep/tstop/ basis_std as thin properties over it, leaving only fit results (theta, Gamma, Sigma_b, mu, forward) as its own state. Behavior is unchanged; the reshaping logic is copied verbatim apart from renaming a shadowing local. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
eval_l2 and the compute_es_metric staticmethod were hosted on NCRFModel but called from exactly one place, _score_mu, and encode cross-validation scoring policy rather than general model behavior. Hosting them on the frozen model leaked CV-specific API onto a class whose outside callers only use eval_obj / explained_variance. Move both to _crossvalidation.py as module functions taking the model (or list of models), and call them directly from _score_mu. Behavior is unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
NCRFModel._from_solver threaded ten loose metadata fields out of RegressionData one by one, and __init__ re-declared all ten. Since the fitted model is what gets persisted to disk (RegressionData is typically too large to store), it needs exactly this small metadata bundle -- but the wide, hand-picked parameter list meant any new reconstruction field had to be threaded through both signatures. Have RegressionData produce the TRFDesign bundle via a trf_design property, and collapse NCRFModel to take a single design object. _from_solver now passes data.trf_design. Behavior is unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The "best mu" selection existed in two places: NCRF._select_mu picked the cross-fit minimum to set the model's mu, and NCRFResult.cv_mu re-derived the same selection independently. cv_info then star-marked rows with an identity (`is`) comparison on floats, which only worked because the same CVResult.mu object happened to be reused. Introduce select_best_mu(cv_results, criterion) as the single criterion -> mu selector, used by both cv_mu and the CV driver. Move the CV orchestration (auto grid, boundary range-extension, ES criterion) out of NCRF into a select_mu() function in _crossvalidation, leaving NCRF._select_mu as a thin dispatcher over the no-CV vs CV cases. Replace the fragile `is` comparison in cv_info with `==`. Behavior is unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Replace the three near-identical __repr__ implementations (NCRFModel, NCRF, NCRFResult) with a shared _orientation_repr helper. In explained_variance, drop the unused enumerate index and the redundant W_meg alias, and use the module logger instead of an ad-hoc logger name. No behavior change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Metrics were free functions taking the whole NCRFModel, each whitening the data and running its own prediction loop; computing both explained_variance and l2_error meant predicting the training set twice. Metrics are now pure functions of observed/predicted per-segment arrays and know nothing about the model. NCRFModel.evaluate() owns whitening and the prediction loop and evaluates a list of metrics on one set of predictions. - drop NCRFModel.explained_variance (thin pass-through) - reuse _predict_whitened in voxelwise_explained_variance - collapse NCRFResult.explained_var into scores['explained_variance'] Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Cross-validation was generic in name but ChampLasso in fact: _score_candidate
unpacked ChampLasso's two-valued objective, and CVResult, select_best_solver,
_extend_mu_grid and _select_es_solver all assumed a mu grid. A second solver
with multiple candidates would have crashed in _score_candidate.
Selection now runs through hooks on Solver, each with a working default, so a
new solver only needs solve():
SolverFit.score() solver-specific scores for a dataset -> {}
Solver.criterion score key to minimize -> 'l2_error'
Solver.select() pick the winner -> min by criterion
Solver.refine() extra candidates for another pass -> ()
Solver.without_history() drop per-iteration storage for folds -> self
Solver.cv_table() render CV scores -> generic table
CVResult.scores is now a dict combining the solver-independent model metrics
with the solver's own, so ChampLasso's mu-grid extension, ES criterion and
score table move into champ_lasso.py where they belong.
NCRFResult.residual is replaced by scores['cross_fit']: the training objective
comes from the same SolverFit.score() hook, removing the ChampLasso-specific
compatibility attribute from the generic result.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
_evaluate_objective returned float | tuple[float, float] depending on a bool flag, forcing callers to know which shape they would get. It now always returns (objective, weighted_l2) and callers unpack what they need. The ChampLassoFit.evaluate_objective wrapper is gone: since selection goes through SolverFit.score(), it was a thin pass-through with no other caller. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The low-rank eigendecomposition of the empirical data covariance and the cholesky-else-pseudo-inverse whitening by sigma_b were each written out twice, in _evaluate_objective and in _ChampLassoState._solve. Both are now helpers: _low_rank_sqrt() and _whiten_by_sigma_b(), the latter whitening any number of arrays and returning the log-determinant its one caller needs. Behavior is unchanged; _evaluate_objective still tries a plain cholesky of Cb first, and _solve still goes straight to the eigendecomposition. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The grid path validated twice (an isinstance sweep, then a float() conversion
in a try/except) to raise the same error; it is now one pass. Number checks go
through a shared _is_number() helper, used by both candidates() and solve(),
rather than repeating the isinstance(Real) / not bool pair.
The isinstance(mu, float) fast path is kept deliberately: it returns self so a
caller can identify the solver it passed to fit(), which replace() would break.
Error messages use the {mu=} form per the project style.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The l1 proximal operator has no callers; ChampLasso.run() composes shrink() with mu * t itself. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
from_data() always centered and scaled in two normalize() calls, each of which copies every covariate array. Only 'spectral' needs two, because its norms are measured on the centered covariates; 'l1' and 'l2' know their factors up front and can apply both steps at once. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
covariate_from_stim() filled the design one row at a time in Python, once per expanded predictor channel. The result is exactly a lower-triangular Toeplitz matrix, X[i, j] = w[i - j]; letting scipy build it is ~9x faster on a 60000-sample, 100-lag channel and produces identical arrays. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The check was a three-deep loop nest in front of the two things the function is named for. Move it to _assert_varying(). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
_solve() nested four deep: segments, Champagne iterations, sources, and the fixed/free orientation branch. Move the source sweep into _update_gamma(), leaving _solve() with the per-segment setup and the iteration loop. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Move the basis projection, which tracked its own column cursor across two loops, into _project_basis(), and collect the segment checks into one if/elif chain. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
_assert_varying() read NDVar.x, which is in the predictor's own dimension order, and reduced over the last axis. For a predictor stored as (time, feature) it therefore compared the feature channels against each other at every time point: two channels that happen to coincide at a single sample were reported as constant over time, which for sparse predictors (impulse trains, where all channels are 0 at most samples) is practically guaranteed. A genuinely constant channel in that layout was missed and only surfaced later as an invalid scaling factor. Ask for the data with time last, as covariate_from_stim() does. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
fit_model() is documented as taking whitened data and is the public single-fit primitive, but it did not check. The solver assumes isotropic noise, so a raw dataset produced wrong coefficients without any error. fit() and evaluate() already guard this. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
predict() returns predictions in M/EEG units unless whitened=True; only the metrics are computed in whitened sensor space. The claim sat right under an example calling predict(), and contradicted the guide. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The two copies of the criterion list contradicted each other: cv_mu() claimed the local l2 minimum with the *smallest* mu, while select_by_criterion() picks the one with the largest mu. Also document the fallback to the global l2 minimum when there is no local one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The `selected` parameter was always the receiver: the only call site, NCRFFit.cv_info(), passed self.solver twice. Drop it and document that cv_table() is called on the configuration search() selected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The configuration cross-validation settled on is `result.solver`, so `result.solver.mu` already answers the common question. cv_mu() was the one ChampLasso-specific method on the otherwise solver-agnostic fit report, and reaching for it forced a function-local import of a solver internal. Its remaining use, asking what a *different* criterion would have picked, is `ncrf._solvers.champ_lasso.select_by_criterion()`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The five store_* booleans, QUANTITIES and without_history() each had to be kept in sync by hand, and a quantity's name existed in three places. Replace them with a single `store` sequence of names, validated in solve(), so QUANTITIES is the one source of truth. record() now rejects names outside QUANTITIES instead of dropping them silently, which turned a misspelled keyword into a no-op. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
RegressionData and ForwardModel are only referenced in annotations, which `from __future__ import annotations` already defers; move them under TYPE_CHECKING like the neighbouring imports, so _forward no longer imports _data at runtime. Also record why _evaluate_objective() tries Cholesky first while _solve() goes straight to the rank-revealing factor -- in master this difference was produced by a hardcoded `raise` inside a `try`, so it now reads as accidental. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Checking it in solve() meant a misspelled quantity only surfaced after a full cross-validation search, since the folds run with store=(). The value is a fixed set of names, so it can be checked as soon as the solver is configured. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
New classes to separate solving and result representations