Repository navigation
Refuse degenerate fits in the AMICA wrapper (#50) - #55
Merged
neuromechanist merged 2 commits intoJul 7, 2026
Merged
neuromechanist merged 2 commits into
neuromechanist merged 2 commits into
Conversation
AMICA.fit() marked is_fitted_ = True unconditionally, so transform()/ get_mixing_matrix()/get_unmixing_matrix() ran on a degenerate fit (stop_reason nan_ll/singular_ll) and returned NaN sources with no error, while state_dict()/save() already refused such a model. Make the wrapper's contract consistent: - fit() sets is_fitted_ only when the fit converged, and exposes converged_ (bool) and stop_reason_ (str) for inspection; a degenerate fit logs a wrapper-level warning. - A new _check_usable() guard raises a clear degenerate error (naming the stop_reason) from transform/get_mixing/get_unmixing/save, mirroring state_dict()'s refusal. An unfitted model still raises a distinct "must be fitted" error. - load() carries stop_reason_/converged_ through (a saved model is always converged, since state_dict refuses degenerate ones). Tested: 3 new wrapper tests (converged/stop_reason exposed on a normal fit; unfitted vs degenerate error messages; transform/get_*/save all refuse a degenerate model) following the established real-fit-then-force-stop_reason pattern; 10 wrapper tests pass. ruff clean; ty no new diagnostics.
PR review (3 Sonnet reviewers): - CRITICAL (regression I introduced): _check_usable keyed on model_ is None, but fit() assigned self.model_ before training, so a mid-fit exception left a half-built backend that the guard let through (and, on refit, stale is_fitted_=True). fit() now trains a LOCAL backend and only publishes it to self on success -- a first-fit crash keeps model_ None (clean "not fitted"), a failed refit keeps the last good model. - Strengthened the degenerate test to a REAL divergence: a single NaN injected into the real EEG forces an actual nan_ll stop (error-path robustness test, not a parity oracle), so it exercises fit()'s real is_fitted_=False/warning bookkeeping instead of a forced marker, and now also covers fit_transform. - Added load() converged_/stop_reason_ round-trip assertions. 10 wrapper tests pass; ruff clean; ty no new diagnostics.
neuromechanist
deleted the
50-amica-wrapper-marks-is_fitted_-even-on-a-degenerate-nan_llsingular_ll-fit
branch
July 7, 2026 05:26
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.
Closes #50. Supersedes #54 (auto-closed when its stacked base, the #51 branch, was deleted on merge of #53). Review was completed on #54 (3 Sonnet reviewers) and all findings addressed — see the #54 review-response comment. Now rebased directly onto main (#50-only diff).
Problem
AMICA.fit()setis_fitted_ = Trueunconditionally, sotransform()/get_mixing_matrix()/get_unmixing_matrix()ran on a degenerate fit (stop_reasoninnan_ll/singular_ll) and returned NaN sources with no exception, whilestate_dict()/save()already refused such a model.Fix — consistent, fail-loud contract (mirrors
state_dict)fit()trains a local backend and only publishes it toselfon success, then setsis_fitted_only when the fit converged; it exposesconverged_(bool) andstop_reason_(str) and logs a wrapper-level warning on a degenerate fit. Publishing only on success means a mid-fit exception leavesselfuntouched (first fit →model_ is None; refit → last known-good model), closing the review's critical finding._check_usable()guard raises a clear degenerateRuntimeError(naming thestop_reason) fromtransform/get_mixing_matrix/get_unmixing_matrix/save; an unfitted model still raises a distinct"must be fitted"ValueError.load()carriesstop_reason_/converged_through.Contract chosen per maintainer decision: expose attributes + refuse output (not a hard
fit()error), so a diverged run stays inspectable.Tests
Real sample EEG only.
test_degenerate_fit_refuses_outputinjects a single NaN into the real data to force a genuinenan_lldivergence (an error-path robustness test, not a parity oracle), exercising the realfit()bookkeeping (is_fitted_=False, the warning) andfit_transform;transform/get_*/saveall refuse. Plus normal-fit attribute exposure, unfitted-vs-degenerate error distinction, andload()round-trip of the new attributes. 10 wrapper tests pass; ruff clean;tyno new diagnostics.