Skip to content

Adaptive PDF selection for AMICATorchNG (#26) - #52

Merged
neuromechanist merged 9 commits into
mainfrom
26-adaptive-pdf-selection-for-amicatorchng
Jul 7, 2026
Merged

Adaptive PDF selection for AMICATorchNG (#26)#52
neuromechanist merged 9 commits into
mainfrom
26-adaptive-pdf-selection-for-amicatorchng

Conversation

@neuromechanist

Copy link
Copy Markdown
Member

Summary

Implements adaptive-PDF selection for AMICATorchNG (issue #26): all five amica15.f90
source-density families plus the extended-Infomax kurtosis auto-switcher. pdftype=0
(generalized Gaussian) stays the default and is byte-for-byte unchanged.

Key correction: the oracle exists

The prior investigation concluded #26 had "no runnable oracle." That read the wrong Fortran
file. The repo shipped amica17.f90 (GG-only) as its reference source, but the validation
binary is amica15mac, whose source is amica15.f90, which does implement the density
families. amica17.f90 is a later GG-only trim; the binary was never amica17. This PR copies
the ground-truth amica15.f90 / amica15_header.f90 into pyAMICA/, so the families are a
bit-comparable parity target rather than a beyond-parity feature.

Families (Fortran pdftype interface)

pdftype family structure score fp
0 generalized Gaussian (default) mixture, rho adapts ρ·sign(y)·|y|^(ρ−1)
2 Gaussian mixture y
3 logistic (sech²) mixture tanh(y/2)
4 sub-Gaussian cosh+ single component y − tanh(y)
1 extended-Infomax adaptive single component per-source y ± tanh(y)

pdftype=1 is Fortran's do_choose_pdfs trigger: each source is flipped between the
super-Gaussian (code 1) and sub-Gaussian (code 4) cosh densities by kurtosis sign on the
kurt_start/num_kurt/kurt_int schedule (documented in runamica15.m as "for ext.
infomax"). For non-GG families rho is frozen (amica15.f90:3682) and the single-component
families 1/4 require n_mix=1.

Validation

  • Density math bit-exact: z0 and fp reproduce the literal amica15.f90 expressions
    to ~1e-15 for every family (deterministic unit tests, no binary needed).
  • Converged LL parity vs amica15mac (real sample EEG, Newton matched): GG 0.0047,
    Gaussian 0.0030, logistic 0.0025, sub-G 0.0050, super-G 0.00006 — all within the GG-parity
    bar. The cosh families' Newton curvature isn't always positive-definite, so NG falls back to
    natural gradient exactly as Fortran does; LL still reaches parity.
  • Default unchanged: pdftype=0 takes a fast path (_pdtype_h returns None) that is
    byte-for-byte the pre-Adaptive-PDF selection for AMICATorchNG #26 code; the existing 39 NG tests pass.
  • The dynamic switch is dead code even in amica15.f90 (m2sum/m4sum are never
    accumulated), so the auto-switcher has no bit-exact oracle and is validated by real-data
    log-likelihood (finite, non-decreasing). This is documented, not hidden.

Tested

  • pyAMICA/tests/torch_tests/test_ng_pdf_families.py: formula parity, GG bit-identity,
    construction validation, per-family real-data fits, auto-switcher schedule/stability, and an
    opt-in binary integration test behind AMICA_RUN_FORTRAN=1.
  • Full tests/torch_tests/ suite green.

Notes for review

  • EEG independent components are characteristically super-Gaussian, so on the sample data the
    switcher keeps all sources at code 1; the sub-Gaussian density is separately covered by the
    fixed pdftype=4 tests (no synthetic data was fabricated to force a sub-Gaussian source).
  • ADR: .context/decisions/0002-adaptive-pdf-families.md.

@neuromechanist neuromechanist linked an issue Jul 6, 2026 that may be closed by this pull request
@neuromechanist

Copy link
Copy Markdown
Member Author

PR review (4 agents, model=Sonnet) — findings addressed

Ran code, silent-failure, test-coverage, and comment reviewers. All non-false-positive findings are fixed in follow-up commits.

Critical

  • Save/load silently dropped the density family (code + test reviewers). state_dict()/from_state_dict() omitted pdftype/kurt_*/pdtype/n_kurt_done, so a reloaded non-GG (or adaptive) model reverted to generalized Gaussian. Fixed: added them to config/_PARAM_TENSORS/extra, restored on load, bumped format_version 1→2, and added test_state_dict_roundtrips_pdftype_state.

Important

  • Kurtosis switch could silently pick a family from a dead model or NaN (silent-failure). Split the decision into a pure _pdtype_from_kurtosis(kurt, nsub) that keeps the prior pdtype and logs when kurtosis is non-finite or the model has zero mass, mirroring the _update_parameters guards. Unit-tested with a constructed mixed-sign/NaN/dead tensor (test_pdtype_from_kurtosis_decision) — this also covers the sub-Gaussian (code 4) branch that real EEG rarely triggers.
  • No validation of kurt_int/kurt_start/num_kurt (silent-failure + test): kurt_int=0 would ZeroDivisionError deep in fit(). Added constructor validation (mirrors the rejint checks) + test_kurt_schedule_validation.
  • Newton + non-GG never tested in default CI (test): added test_family_fit_with_newton. (Newton with non-GG families is what Fortran itself does — its posdef fallback fires on the cosh families too — and the opt-in binary parity test confirms convergence, so this is validated behavior, not new territory.)
  • Added test_multimodel_fixed_family and test_adaptive_switch_with_rejection to cover the per-model and post-rejection paths.

Comments/docs

  • Fixed a citation that pointed at amica15.f90 line numbers from inside an amica17.f90-scoped docstring (the two sources are not interchangeable); added the amica15.f90 line to _LOG4; corrected a 1327→1328 citation; documented the four new constructor params in the class docstring; added a module-docstring pointer explaining the amica15-vs-amica17 split; and pasted the runamica15.m "ext. infomax" excerpt into ADR 0002 so the switcher's provenance is verifiable in-repo.
  • Reworded two test docstrings that claimed "monotone" — the assertion checks net non-decrease (last ≥ first), which is what natural-gradient AMICA actually guarantees.

Intentionally not changed

  • ufp / y unguarded at y == 0 (silent-failure, MEDIUM): this is pre-existing on the GG path, out of scope for Adaptive-PDF selection for AMICATorchNG #26, and adding a guard would perturb the default pdftype=0 trajectory that must stay bit-identical. y = beta*(b - mu) is never exactly 0 on real (continuous) data, and all parity tests pass. Left as-is; a separate hardening issue is the right home if we want it.

@neuromechanist
neuromechanist merged commit 5c12548 into main Jul 7, 2026
5 checks passed
@neuromechanist
neuromechanist deleted the 26-adaptive-pdf-selection-for-amicatorchng branch July 7, 2026 00:15
neuromechanist added a commit that referenced this pull request Aug 10, 2026
Added tests that the five new config keys (use_min_dll/min_dll/
maxincs/use_grad_norm/min_nd) round-trip through state_dict()/
from_state_dict(), and that a simulated pre-#207 payload (format_
version 3, missing those keys) still loads and falls back to the
Fortran defaults. Also documented, at the format_version check itself,
why it deliberately was not bumped for this change (prior precedent
#52/#53 bumped it; the additive-only new keys don't need to).

PR #213 review finding 6.
neuromechanist added a commit that referenced this pull request Aug 10, 2026
Added tests that the five new config keys (use_min_dll/min_dll/
maxincs/use_grad_norm/min_nd) round-trip through state_dict()/
from_state_dict(), and that a simulated pre-#207 payload (format_
version 3, missing those keys) still loads and falls back to the
Fortran defaults. Also documented, at the format_version check itself,
why it deliberately was not bumped for this change (prior precedent
#52/#53 bumped it; the additive-only new keys don't need to).

PR #213 review finding 6.
neuromechanist added a commit that referenced this pull request Aug 10, 2026
* feat: add NG convergence stops (issue #207)

AMICATorchNG was missing three Fortran convergence criteria
(amica15.f90): use_min_dll/maxincs (consecutive small-gain stop),
use_grad_norm/min_nd (weight-gradient-norm stop), and the
decrease-branch's ".or. ndtmpsum <= min_nd" half -- the last is
why lrate could sit at newtrate and oscillate under do_newton=True
without ever stopping. All three are Fortran-faithful defaults
(True/1e-9/5/True/1e-7, matching amica15_header.f90).

ndtmpsum is now computed every iteration (Fortran-faithful,
including the comp_used mask): the direction/dAk computation in
_update_parameters was decoupled from the share_comps A-freeze
gate, since Fortran computes dAk/ndtmpsum unconditionally in
accum_updates_and_likelihood, strictly before the separately-gated
update_A step. Default (non-sharing) path is unaffected.

New stop_reason values (min_dll, grad_norm, grad_norm_floor) are
converged, not degenerate; plumbed through AMICA (**kwargs) and
state_dict()/from_state_dict(). Corrected a misleading comment
claiming only amica17 normalizes LL before the min_dll comparison
-- amica15 (the actual reference binary's source) normalizes
identically; the real divergence is numpy_impl's un-normalized
raw-sum LL comparison, a separate pre-existing gap this does not
touch.

Tested: full torch suite green (188 passed, 5 pre-existing skips);
validate_implementations.py unchanged at max-iter 100 and 2000
(matches sample_params.json's budget) -- PyTorch LL/iteration count
identical before/after, neither stop fires on the bundled 32-channel
sample within that budget.

* test: add NG convergence-stop suite (issue #207)

Real bundled sample EEG only. Covers: each stop firing with the
right stop_reason (min_dll, grad_norm, grad_norm_floor, and the
pre-existing lrate_floor unshadowed by the new check); the maxincs
consecutive-count rule including reset-on-larger-gain, verified
against an independent reimplementation applied to a stops-disabled
reference trajectory rather than hardcoded iteration numbers; the
have_prev guard (never fires before two LL values exist); the
share_comps freeze window still computing a fresh (non-stale)
ndtmpsum every iteration; a converged stop leaving transform/
state_dict/AMICA.save usable; keep_best and do_reject interactions;
and that both stops disabled reproduces pre-#207 behavior (never
emits the three new stop_reason values).

14/14 pass; ruff and ty clean.

* test: drop slow marker from non-Fortran convergence tests

Three tests in test_ng_convergence.py used only bundled EEG and pure
PyTorch (no Fortran binary) and ran in seconds, but were marked
@pytest.mark.slow. CI runs pytest -m "not slow", documented as
excluding tests that invoke the macOS-only Fortran reference binary,
so these three never ran in CI. One of them,
test_a_frozen_window_still_computes_fresh_grad_norm, is the only test
of the dAk/A-freeze decoupling (issue #207), so that change had zero
CI coverage.

PR #213 review finding 1.

* fix: document and test stop_reason shadowing (issue #207)

None of the three fit()-loop stop blocks (decrease branch; min_dll;
grad_norm) short-circuits on an earlier one having already fired the
same iteration, matching Fortran's independent leave=.true. structure
(not a fidelity bug). But the standalone grad_norm check runs
unconditionally after the decrease branch, so under the shipped
use_grad_norm=True default it always wins: "grad_norm_floor" is
unreachable as a final stop_reason, and the use_grad_norm docstring
wrongly implied it was the fix for the reported CUDA case. Corrected
the docstring/comments in torch_impl/core.py and amica.py, and added a
test proving the shadowing under shipped True/True defaults.

PR #213 review finding 2.

* test: rename mislabeled do_reject test, add missing coverage

test_do_reject_interaction_min_dll_stop_leaves_good_idx_usable set
use_min_dll=False, so it actually exercised grad_norm_floor, not
min_dll; renamed to match. Added the two genuinely missing do_reject
combinations: the standalone min_dll and grad_norm stops.

PR #213 review finding 3.

* test: exercise a genuine keep_best overshoot restore

The old test's trajectory was monotonically increasing, so
final_ll_ == max(ll_history) == ll_history[-1] held whether the
restore logic worked or was a no-op. Reworked it around the known
non-monotone recipe from test_write_amica_output_ll_matches_kept_iterate
(#92), combined with a loosened min_dll so the run stops a few
iterations past its peak via the new min_dll stop_reason, and assert
final_ll_ != ll_history[-1] to prove the restore branch actually ran.

PR #213 review finding 4.

* test: exercise AMICA.save/load in convergence-stop tests

The wrapper usability test's own docstring claimed "transform()/
save() usable" but never called AMICA.save() anywhere in the file.
Added a real save()/load() round trip and confirmed the reloaded
model reports the same stop_reason_ and reproduces transform()
exactly.

PR #213 review finding 5.

* test: cover issue #207 config persistence round-trip

Added tests that the five new config keys (use_min_dll/min_dll/
maxincs/use_grad_norm/min_nd) round-trip through state_dict()/
from_state_dict(), and that a simulated pre-#207 payload (format_
version 3, missing those keys) still loads and falls back to the
Fortran defaults. Also documented, at the format_version check itself,
why it deliberately was not bumped for this change (prior precedent
#52/#53 bumped it; the additive-only new keys don't need to).

PR #213 review finding 6.

* test: add stop reachability at literal shipped default thresholds

Every prior min_dll/grad_norm test loosens the threshold by 5-6 orders
of magnitude to force a fast stop, which would not catch a scale bug
in the comparison itself (issue #212 found exactly that in the
numpy_impl backend). Added a fast (a few seconds), non-slow test at
the literal shipped defaults (min_dll=1e-9, maxincs=5,
use_grad_norm=True, min_nd=1e-7, none overridden) that reaches
min_dll via an early Newton start on a small real-data subset.

PR #213 review finding 7.

* test: cover mir_history_ vs keep_best restore and save/load

Issue #161 flagged two documented-but-untested mir_history_ claims:
that a keep_best restore (#51) does not rewrite it (so its last entry
can be from discarded, pre-restore parameters, distinct from
model.mir(X) on the returned ones), and that it comes back empty after
a save/load round trip (not persisted in state_dict()). Both verified
true on real data; also updated the module docstring to summarize the
full set of PR #213 review additions in this file.

Folds in issue #161.

* docs: add changelog entry for issue #207 convergence stops

* fix: correct timing claim in threshold-reachability docstring

The test is actually the slowest in the file (~8s from the 326-iteration
fit itself), not "well under 3s" as originally written; corrected to
state the real number and why it's still not a slow-marker candidate.

* Give the shipped-default reachability test real headroom

The iteration at which min_dll fires is BLAS-dependent: 326 on
macOS-arm64, 412 on Linux-x86_64 with a CUDA torch build, and past 500
on the GitHub Linux runner, where CI failed with stop_reason=max_iter.
At max_iter=500 the test consumed 82 percent of its budget on the
fastest platform, so any numerical variation tipped it over.

The claim under test is that the default threshold is reachable at all,
not that it is reached by a given iteration, so raise the budget well
above the observed spread and record the spread in the docstring.

Refs #207

* Assert convergence behaviour, not the iteration it happens on

CI failed twice on iteration-count assumptions. First the stop reason,
because max_iter=500 left no headroom; then a leftover
len(ll_history) < 500 bound after the budget was raised. The stop fires
at 326 on macOS-arm64, 412 on Linux-x86_64 with a CUDA torch build, and
1076 on the GitHub runner, so any constant fitted to one machine is a
trap.

Both bounds now track the budget. The unrelated len(ll_history) == 23
golden value in the lrate_floor test is loosened for the same reason,
before it fails the same way.

Refs #207

* Rebuild paper.pdf [skip ci]

---------

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
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.

Adaptive-PDF selection for AMICATorchNG

1 participant