Skip to content

Implement save/load persistence for AMICATorchNG - #44

Merged
neuromechanist merged 3 commits into
mainfrom
36-implement-saveload-persistence-for-amicatorchng
Jul 6, 2026
Merged

Implement save/load persistence for AMICATorchNG#44
neuromechanist merged 3 commits into
mainfrom
36-implement-saveload-persistence-for-amicatorchng

Conversation

@neuromechanist

Copy link
Copy Markdown
Member

Closes #36. Also closes the save/load half of #15 (test coverage).

Problem

AMICA.save() / AMICA.load() raised NotImplementedError after #32 removed the old AMICATorch state_dict path. There was no way to persist a fitted PyTorch model.

Change

  • AMICATorchNG.state_dict() serializes the model as three parts: config (constructor args, dtype stored by name), params (fitted tensors moved to CPU: A/W/c/mu/alpha/beta/rho/gm/comp_list/mean/sphere), and extra (sldet, iteration, ll_history, optimizer state). Every value is a tensor or plain Python primitive, so the payload round-trips through torch.save/torch.load with weights_only=True (no custom classes or torch.dtype objects).
  • AMICATorchNG.from_state_dict(state, device=None) rebuilds the object via the constructor and restores the tensors/scalars. Raises on an unfitted state_dict() and on an unknown format_version.
  • AMICA.save() writes the wrapper config + backend state; AMICA.load() is now a classmethod returning a fitted, transform-ready model. The MPS/float64 device fallback used by fit() was factored into _select_device() and reused by load().

Tests (real sample EEG, no mocks)

Replaces the two NotImplementedError placeholder tests with:

  • test_ng_save_load_roundtrip - fit -> save -> load reproduces mixing/unmixing matrices and transform() output exactly (lossless CPU float64 restore).
  • test_ng_save_requires_fit - saving an unfitted model raises.
  • test_ng_load_rejects_unknown_version - a tampered format_version fails loudly (no silent-failure).

Full torch suite: 26 passed.

Notes

  • No backward-compat shim for the old (never-working) format.
  • load() changed from an instance method to a classmethod (the old body only raised, so no callers relied on the instance form).

AMICATorchNG.state_dict()/from_state_dict() serialize config + fitted
tensors as plain tensors/primitives (dtype by name) so the payload
round-trips through torch.save/load with weights_only=True. AMICA.save()
writes it; AMICA.load() is a classmethod that rebuilds a transform-ready
model with the same MPS/float64 device fallback as fit(). Replaces the
NotImplementedError stubs (closes #36) and the placeholder tests with a
real fit->save->load->transform round-trip on sample EEG (#15).
@neuromechanist neuromechanist linked an issue Jul 6, 2026 that may be closed by this pull request
Review findings (4 Sonnet reviewers):
- silent-failure: state_dict() refuses to serialize a degenerate model
  (stop_reason nan_ll/singular_ll) or one with non-finite params, so a NaN
  model can no longer round-trip silently into NaN sources. Added descriptive
  errors for malformed payloads (missing sections/keys) and A/comp_list
  shape-drift guards in _load_params.
- code-review: state_dict() now .clone()s param tensors so the snapshot is
  independent of the live model (fit() mutates A/mu/beta in place); an aliased
  CPU snapshot would silently roll forward if captured mid-fit.
- test-coverage: added backend-level round-trip asserting all 11 param tensors
  and every extra field with do_reject+do_newton (exercises mu/alpha/beta/rho/
  gm and the good_idx branch), plus unfitted/degenerate/non-finite/snapshot
  guard tests.
- comments: corrected _PARAM_TENSORS scope, state_dict 'optimizer state', and
  the test's lossless-restore comment.
@neuromechanist

Copy link
Copy Markdown
Member Author

Review pass (4 Sonnet reviewers) — all findings addressed

Ran the pr-review-toolkit reviewers (code, silent-failure, test-coverage, comments) on the diff. Every material finding was addressed in commit 8746236; no false positives skipped.

Silent-failure (critical): a fit that ended on a non-finite log-likelihood (stop_reason nan_ll/singular_ll) round-tripped through save()/load() with no complaint, and transform() then returned all-NaN.

  • state_dict() now refuses to serialize a degenerate model, and a defense-in-depth torch.isfinite scan over the param tensors blocks any non-finite parameter even if stop_reason bookkeeping misses it.
  • Malformed payloads (missing wrapper/backend/config/params/extra sections or param keys) raise descriptive ValueErrors naming the file/section instead of a bare KeyError.
  • _load_params guards A/comp_list shape drift vs the rebuilt config.

Code-review (important): state_dict() used .detach().cpu(), which aliases live storage when the model is already on CPU (the default). Since fit() mutates A/mu/beta in place per iteration, a snapshot captured mid-fit would silently roll forward. Now .clone()d so the snapshot is independent (verified by test_state_dict_snapshots_not_aliases). Config coverage, annealed-vs-original param handling, device/dtype restoration, and weights_only=True safety were all confirmed correct.

Test-coverage: the round-trip only exercised 6 of 11 param tensors. Added a backend-level round-trip asserting every param tensor and every extra field with do_reject+do_newton active (so mu/alpha/beta/rho/gm and the good_idx branch are non-trivial), plus unfitted / degenerate / non-finite / snapshot guard tests.

Comments: corrected _PARAM_TENSORS scope claim, the state_dict "optimizer state" wording, and the test's lossless-restore comment.

Full torch suite: 31 passed (26 baseline + 5 new persistence tests). Ruff clean.

@neuromechanist
neuromechanist merged commit 838e633 into main Jul 6, 2026
5 checks passed
@neuromechanist
neuromechanist deleted the 36-implement-saveload-persistence-for-amicatorchng branch July 6, 2026 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement save/load persistence for AMICATorchNG

1 participant