Skip to content

Commit 39d732b

Browse files
Epic polish: audit fixes (#316)
* Attribute degenerate-fit refusal to AMICA wrapper Row 5 of the differences table credited the raw PyTorch backend with refusing transform/get_*/save on a degenerate fit; that guard is the AMICA wrapper's _check_usable contract. The raw AMICATorchNG backend has no such guard of its own (tracked as issue #306). * Document unmapped Fortran keywords Add a section resolving the page's exhaustiveness contract: points to fortran_params.py's FORTRAN_UNSUPPORTED_KEYS as the enumeration, names filter_length/dft_length/decwindow as dead in the reference itself (alongside the already-documented do_choose_pdfs/Spinv2 dead code), and records the do_rho-vs-pdftype divergence: Fortran can freeze the GG shape while keeping pdftype=0, but every pamica backend derives the freeze from pdftype directly, making that combination unreachable. Links issues #312/#314 for the live lifecycle/diagnostics gaps. * Port variance_order to AMICAMLXNG Adds the EEGLAB back-projected-variance component order (issue #92) to the MLX backend, mirroring AMICATorchNG.variance_order with MLX's model-major W layout ((n_models, n, n)) and the unfitted RuntimeError. Closes the one accessor gap the epic's feature-parity audit found in an otherwise-complete Phase 3; updates the module docstring's ported-features claim to match. Adds cross-backend parity coverage against a float64 AMICATorchNG twin holding identical fitted parameters (order matches exactly on a 30-iteration multi-model fit with non-degenerate variance gaps, with a gap-size assertion documenting the tie risk), plus MLX-only unfitted/invalid-model_idx/permutation tests. * Correct validation harness backend coverage claim AGENTS.md claimed the harness runs "both implementations (NG + NumPy)"; validate_implementations.py only exercises torch vs Fortran. NumPy parity lives in pytest (test_sample_data_numpy_vs_fortran); MLX validation lives in mlx_tests/ plus the cross-backend suites. Extending the harness itself is tracked as issue #315. * Fix unsupported-key comment for writestep/do_history The FORTRAN_UNSUPPORTED_KEYS entries for writestep/do_history/ histstep said these have no pamica equivalent; false for the legacy NumPy backend, which implements all three (numpy_impl/core.py's fit loop and _write_history). Keeps the keys unsupported here (this module targets the torch wrapper, which has no matching mechanism) but corrects the reason: NumPy-only, torch/MLX gap tracked as #312. * Refresh stale feature-parity status notes feature_parity.md and progress_summary.md still claimed the runtime- vs-Fortran benchmark was unmeasured and issue #15 (save/load and plot_components test coverage) was open; both landed since. Also corrects progress_summary.md's validation-harness claim (same fix as AGENTS.md: torch-vs-Fortran only, not NG + NumPy). Adds a header note to feature_parity.md pointing to amica-differences.md's table as the live, maintained backend-parity source of truth. * Align NumPy backend's inert pdftype default to 0 self.pdftype is never read after assignment (this backend always runs the GG update; _compute_log_pdf takes no pdftype param), so the old default of 1 was a harmless but confusing mismatch against torch/MLX's constructor default of 0. Aligned for surface consistency only; no behavior change. * Add changelog entry for epic polish round Summarizes the audit-driven fixes ahead of merge to dev: MLX variance_order, the doc corrections, and the new tracking issues (#312, #314, #315) plus the #306 extension. * Scope variance_order precision claim to defaults
1 parent a189c73 commit 39d732b

10 files changed

Lines changed: 292 additions & 30 deletions

File tree

.context/feature_parity.md

Lines changed: 16 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,11 @@
11
# Feature Parity Report: Fortran vs PyTorch AMICA
22

3+
**This file predates the MLX and NumPy backends and covers only the Fortran-vs-`AMICATorchNG`
4+
comparison from issue #24's original parity push; it is not maintained against either backend added
5+
since. The live, maintained backend-parity source of truth is
6+
[`docs/guides/amica-differences.md`](../docs/guides/amica-differences.md)'s "At a glance" table,
7+
which covers all three array backends. Treat this file as a historical snapshot.**
8+
39
Compares the Fortran reference (`amica15.f90`, binary `amica15mac`) with the current PyTorch
410
backend `AMICATorchNG` (the sole PyTorch backend; the earlier Adam/autograd `AMICATorch` and
511
`AMICATorchV2` were removed in #32). The NumPy reference (`AMICA_NumPy`) is retained as an oracle
@@ -8,7 +14,9 @@ and CLI. Living status is in `../AGENTS.md` (Known Issues); ADRs are in `decisio
814
**Headline:** single-model parity is done (LL ~ -3.40 vs Fortran -3.4018; Hungarian component
915
correlation ~0.997 with Newton positive-definite, 0 fallbacks). Adaptive PDF (#26), multi-model
1016
(#27, distributional equivalence), best-iterate (#51), and the degenerate-fit contract (#50) are
11-
done, as is component sharing (#60). The one unverified success criterion is runtime.
17+
done, as is component sharing (#60). Runtime vs Fortran has since been measured and the ~2-3x
18+
criterion met/exceeded (CUDA float64 ~4.5x, MLX ~7x on Apple Silicon; see `AGENTS.md`'s
19+
Performance section and `plan.md`'s checked runtime item) -- the "unmeasured" row below is stale.
1220

1321
## Core AMICA features
1422

@@ -53,7 +61,7 @@ done, as is component sharing (#60). The one unverified success criterion is run
5361
|---------|---------|--------------|--------|-------|
5462
| Binary data loading | yes | yes | Complete | `.fdt` files |
5563
| Parameter files | yes | yes | Complete | JSON |
56-
| Checkpoint save/load | yes | yes | Complete | NG state_dict persistence (#36); untested (#15) |
64+
| Checkpoint save/load | yes | yes | Complete | NG state_dict persistence (#36); tested (#15, closed) |
5765
| Result loading | yes | yes | Complete | via `loadmodout()` |
5866

5967
## Numerical stability
@@ -71,7 +79,7 @@ done, as is component sharing (#60). The one unverified success criterion is run
7179
|---------|---------|--------------|-------|
7280
| OpenMP parallelization | yes | n/a | PyTorch uses tensor ops / device parallelism |
7381
| GPU support | no | yes | CUDA / MPS / CPU automatic (float64 parity runs on CPU/CUDA) |
74-
| Runtime vs Fortran | baseline | **unmeasured** | the 2-3x criterion has never been benchmarked |
82+
| Runtime vs Fortran | baseline | **measured, met/exceeded** | CUDA float64 ~4.5x over 16-thread CPU, MLX ~7x on Apple Silicon (#77/#84); see `AGENTS.md` Performance section |
7583

7684
## Validation status (issue #24)
7785

@@ -88,9 +96,10 @@ indistinguishable from Fortran's run-to-run distribution (#27, `.context/issue-2
8896

8997
## Remaining roadmap
9098

91-
- [ ] **Performance benchmark** vs the Fortran binary (CPU/CUDA/MPS); verify or revise the 2-3x
92-
runtime criterion.
99+
- [x] **Performance benchmark** vs the Fortran binary (CPU/CUDA/MPS); 2-3x runtime criterion
100+
met/exceeded (#77/#84).
93101
- [x] **Component sharing** (`share_comps`, `share_start`/`share_iter`, `comp_thresh`) ported to
94102
`AMICATorchNG` (#60), off by default, behavior-validated on real data.
95-
- [ ] **Test hardening:** single-channel / single-sample edge cases, numerical-stability
96-
regression tests, and `save`/`load` + `plot_components` coverage (#15).
103+
- [x] **Test hardening:** `save`/`load` + `plot_components` coverage landed (#15, closed).
104+
Single-channel/single-sample edge cases and numerical-stability regression tests remain
105+
open; not tracked under #15.

.context/progress_summary.md

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -71,13 +71,17 @@ what remains as of the v0.1.0 preparation.
7171
- UV is the canonical environment (`pyproject.toml` + `uv.lock`); the legacy conda env is retired.
7272
- CI is live and green on `main`: ruff lint/format, pytest (excluding slow/Fortran-binary parity),
7373
and a build + clean-env import matrix on Python 3.12 and 3.13. Typos check green.
74-
- Validation harness (`validate_implementations.py`) runs both backends and matches components via
75-
the Hungarian algorithm, on real sample EEG plus the Fortran binary (`amica15mac`).
74+
- Validation harness (`validate_implementations.py`) runs the PyTorch (NG) backend against the
75+
Fortran binary (`amica15mac`) and matches components via the Hungarian algorithm, on real sample
76+
EEG; it does not exercise the NumPy or MLX backends. NumPy-vs-Fortran parity lives in pytest
77+
(`test_sample_data_numpy_vs_fortran`), MLX validation in `mlx_tests/` plus the cross-backend
78+
suites (extending the harness itself is tracked as issue #315).
7679

7780
## Remaining before / around v0.1.0
7881

79-
- **Performance benchmark:** the "runtime within 2-3x of Fortran" success criterion has never been
80-
measured. Needs a repeatable NG-vs-Fortran benchmark across CPU/CUDA/MPS.
81-
- **Test hardening:** no single-channel / single-sample edge tests, no numerical-stability
82-
regression tests (mincond / minlog / maxdble / mineig bounds); `AMICA.save`/`load` and
83-
`plot_components` remain untested (issue #15).
82+
- **Performance benchmark:** DONE. The "runtime within 2-3x of Fortran" success criterion has been
83+
measured and met/exceeded: CUDA float64 ~4.5x over a 16-thread CPU, MLX ~7x on Apple Silicon
84+
(#77/#84; see `AGENTS.md`'s Performance section).
85+
- **Test hardening:** `AMICA.save`/`load` and `plot_components` coverage landed (issue #15,
86+
closed). Single-channel / single-sample edge tests and numerical-stability regression tests
87+
(mincond / minlog / maxdble / mineig bounds) remain open, not tracked under #15.

AGENTS.md

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -104,8 +104,10 @@ gap vs the PyTorch backend is none, other than float32-only precision (Apple GPU
104104
- PyTorch backend with GPU/MPS/CPU support; the `AMICATorchNG` natural-gradient EM backend now
105105
matches the Fortran reference (LL ~ -3.40, component correlation ~0.997) with Newton enabled
106106
and positive-definite (issue #24).
107-
- Validation harness runs both implementations (NG + NumPy) and matches components via the Hungarian
108-
algorithm.
107+
- `validate_implementations.py` runs the PyTorch (NG) backend against the Fortran binary and matches
108+
components via the Hungarian algorithm; it does not exercise the NumPy or MLX backends. NumPy-vs-
109+
Fortran parity lives in pytest (`test_sample_data_numpy_vs_fortran`), and MLX validation lives in
110+
`mlx_tests/` plus the cross-backend suites (extending the harness itself is tracked as issue #315).
109111
- Newton and exact-EM updates are implemented in `AMICATorchNG` and the legacy NumPy `numpy_impl/core.py`
110112
(both Fortran-faithful). Adaptive PDF (#26) is DONE (all five `pdftype` families + ext-Infomax
111113
switcher); full multi-model matching (#27) is validated by distributional equivalence.

docs/changelog.md

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,32 @@ Release notes are also published on the
55

66
## Unreleased
77

8+
- **Epic #278 polish round: audit-driven fixes ahead of merge to `dev`.**
9+
`AMICAMLXNG` gained `variance_order` (issue #92), the EEGLAB
10+
back-projected-variance component order, closing the one accessor gap the
11+
epic's feature-parity audit found in an otherwise-complete Phase 3
12+
(validated on real data against a float64 `AMICATorchNG` twin holding
13+
identical fitted parameters: the component order matches exactly on a
14+
config with non-degenerate variance gaps). The audit also found several
15+
stale/misattributed doc claims, fixed here: `docs/guides/amica-differences.md`
16+
now attributes the degenerate-fit refusal to the `AMICA` wrapper rather than
17+
the raw PyTorch backend (the raw-backend gap is issue #306), and gained an
18+
"Unmapped Fortran keywords" section pointing to `fortran_params.py`'s
19+
`FORTRAN_UNSUPPORTED_KEYS` as the enumeration, naming three keywords dead in
20+
the reference itself (`filter_length`/`dft_length`/`decwindow`), and
21+
recording the `do_rho`-vs-`pdftype` divergence explicitly; `AGENTS.md` and
22+
`.context/progress_summary.md` no longer claim `validate_implementations.py`
23+
covers the NumPy/MLX backends (it is torch-vs-Fortran only; extending it is
24+
issue #315); `.context/feature_parity.md` and `.context/progress_summary.md`
25+
no longer claim the runtime-vs-Fortran benchmark is unmeasured or that issue
26+
#15 (save/load and `plot_components` test coverage) is open, both stale
27+
since earlier work landed; and `fortran_params.py`'s unsupported-keys table
28+
no longer claims `writestep`/`do_history`/`histstep` have no pamica
29+
equivalent -- they are implemented on the legacy NumPy backend, just not
30+
yet on torch/MLX (issue #312). Also aligned the legacy NumPy backend's
31+
inert `pdftype` constructor default to 0, matching torch/MLX (the value is
32+
never read, so this is a documentation-equivalent surface fix, not a
33+
behavior change).
834
- **MLX backend: outlier rejection, LLt stash, EEGLAB export, MIR/PMI**
935
(issue #289, epic #278 Phase 3 -- completes the epic). `AMICAMLXNG` gains:
1036
the LLt stash (issue #157), filled per-block by the E-step (never a second

docs/guides/amica-differences.md

Lines changed: 55 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ that is not listed, that is a bug worth
1616
| 2 | Zero numerical rank | `numeigs = 0`, continues | `ValueError` naming cause and fix | fitting a zero-dimensional model is not a recoverable state | — (no reason to want it) |
1717
| 3 | Returned iterate | last EM iterate | highest-likelihood iterate (`keep_best`) | the lrate schedule is non-monotone; late Newton overshoots cut LL variance 12.7x → 2.0x | `keep_best=False` |
1818
| 4 | Newton | on (`do_newton=1`) | off | isolates the algorithm from initialization for parity work | `do_newton=True` |
19-
| 5 | Degenerate fits | returns NaN sources, and writes them out on its `writestep` cadence | PyTorch refuses `transform`/`get_*`/`save`; NumPy reports `converged=False` with a `stop_reason`, refuses the final write, and skips each periodic checkpoint with a logged reason (leaving the last valid one on disk) | NaN sources silently poison downstream analysis, and `loadmodout` reads a NaN checkpoint back without complaint | — (see issues #50 and #240) |
19+
| 5 | Degenerate fits | returns NaN sources, and writes them out on its `writestep` cadence | the `AMICA` wrapper refuses `transform`/`get_*`/`save` (the raw `AMICATorchNG` backend has no such guard of its own — tracked as issue #306); NumPy reports `converged=False` with a `stop_reason`, refuses the final write, and skips each periodic checkpoint with a logged reason (leaving the last valid one on disk) | NaN sources silently poison downstream analysis, and `loadmodout` reads a NaN checkpoint back without complaint | — (see issues #50 and #240) |
2020
| 6 | Precision | float64 | float64 (float32 on Apple GPUs) | Apple GPUs have no float64; float32 agrees to ~7 significant digits, not bit-parity | `dtype=torch.float64` |
2121
| 7 | Sensor-space maps | `Spinv` applied internally | `get_sensor_mixing_matrix()` | `get_mixing_matrix()` returns sphered-space `A`; switching its meaning by data conditioning would be worse ||
2222
| 8 | Columns merged away by `share_comps` | updated to NaN, then hidden by the `comp_used` mask | frozen at their last finite value (never divided) | a fit must not end holding NaN parameters, mask or no mask; the columns are dead either way | — (see issues #60, #240) |
@@ -589,3 +589,57 @@ Practical notes:
589589
can appear on disk mid-fit. The final write replaces it — what is on disk when
590590
`fit` returns is the winner's state, which is a tested claim, not just a
591591
documented intention.
592+
593+
## Unmapped Fortran keywords
594+
595+
This page's own contract ("anything not on this page is intended to match the
596+
reference") only covers *behavior*. A Fortran keyword that pamica's parameter
597+
translator does not map at all is a separate, narrower question, and it has
598+
its own enumeration: `pamica/fortran_params.py`'s `FORTRAN_UNSUPPORTED_KEYS`
599+
lists every keyword the reference parser accepts that this project does not
600+
translate into a pamica constructor/`fit()` argument, with a one-line reason
601+
for each. That dict, not this page, is the exhaustive list.
602+
603+
Most of those keywords are unmapped because the feature genuinely has no
604+
pamica equivalent (checkpoint warm-start, per-family EM freeze toggles,
605+
console/file-reporting cadence, ...). Three, though, are dead even in the
606+
reference binary itself — the same category as the `do_choose_pdfs` and
607+
`Spinv2` dead code documented above:
608+
609+
- **`filter_length`, `dft_length`** — parsed, printed, and broadcast to every
610+
MPI rank (`amica15.f90` `case('filter_length')`/`case('dft_length')`,
611+
~3263-3280), but no filter or DFT subroutine exists anywhere in
612+
`amica15.f90`/`funmod2.f90` to consume either value. A pre-filtering
613+
pipeline was apparently planned and never built.
614+
- **`decwindow`** — parsed into the header the same way, but its
615+
`MPI_BCAST(decwindow, ...)` call is commented out (`amica15.f90:192`), so a
616+
multi-process reference run never even shares the parsed value past rank 0;
617+
every worker rank keeps the compiled-in default regardless of what the
618+
parameter file said.
619+
- (Already documented above:) `do_choose_pdfs`'s `pdftype=1` auto-switcher is
620+
declared but its moment buffers (`m2sum`/`m4sum`) are never accumulated, and
621+
`share_comps`'s `Spinv2` similarity metric is declared but never allocated.
622+
623+
One live (non-dead) divergence is worth recording explicitly rather than
624+
leaving it implicit in the unsupported-keys table: Fortran's `do_rho` can
625+
freeze the generalized-Gaussian shape parameter while keeping `pdftype=0`
626+
an independent toggle, "use the GG family but do not adapt its shape this
627+
run." Every pamica backend instead hard-derives the freeze from `pdftype`
628+
itself: `AMICATorchNG`/`AMICAMLXNG` set `self.dorho = (pdftype == 0)`
629+
directly (so `pdftype=0` always updates rho, never frozen), and the legacy
630+
NumPy backend has no `pdftype` family switch at all — it always runs the
631+
GG update, the same effective always-on freeze-off behavior. So the
632+
combination Fortran supports (GG family, frozen shape) is unreachable on any
633+
current pamica backend. Restore-reference: none currently exposed; `do_rho`
634+
would need to become an independent constructor keyword to port this on
635+
demand.
636+
637+
Two clusters of keywords are neither dead code nor "no pamica equivalent" —
638+
they are live Fortran features with a real gap, filed rather than silently
639+
carried:
640+
641+
- **Lifecycle** (checkpoint warm-start `load_*`, `writestep`/`do_history`/
642+
`histstep` periodic checkpointing on torch/MLX, per-family EM freeze
643+
toggles, `fix_init`): issue #312.
644+
- **Diagnostics** (`write_nd`'s per-component gradient trajectory, the
645+
per-iteration run log): issue #314.

pamica/fortran_params.py

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -61,8 +61,11 @@
6161
6262
``FORTRAN_UNSUPPORTED_KEYS`` lists the remaining 32 keywords the Fortran
6363
parser accepts (checkpoint warm-start, per-family EM freeze toggles, FIR/DFT
64-
pre-filtering, console/file reporting cadence, ...) that have no pamica
65-
equivalent; these are
64+
pre-filtering, console/file reporting cadence, ...) that this module does not
65+
translate -- most with no pamica equivalent anywhere, but a few
66+
(``writestep``/``do_history``/``histstep``) exist on the legacy NumPy backend
67+
only (see the table entry below); this module's output targets the torch
68+
wrapper, which has no matching mechanism (issue #312). These are
6669
*deliberately* unmapped, not missed, and ``read_fortran_param_file`` warns
6770
about them by name rather than dropping them in silence. A keyword absent
6871
from *both* dicts is not a Fortran keyword this module knows about at all
@@ -300,9 +303,16 @@
300303
"print_debug": "Fortran console verbosity, no pamica equivalent",
301304
"write_nd": "Fortran output-file toggle (weight/gradient dumps)",
302305
"write_LLt": "Fortran output-file toggle (per-sample log-likelihood dumps)",
303-
"writestep": "Fortran output-file write cadence",
304-
"do_history": "Fortran periodic weight-history dump",
305-
"histstep": "Fortran periodic weight-history dump cadence",
306+
# Periodic on-disk checkpointing DOES have a pamica equivalent, but only
307+
# on the legacy NumPy backend (numpy_impl/core.py's fit loop, ~2195-2198,
308+
# and _write_history, ~2460-2489), which implements writestep/do_history/
309+
# histstep with the same names and cadence semantics as Fortran. These
310+
# stay unsupported HERE only because this module's output targets the
311+
# torch wrapper (AMICA.fit/AMICATorchNG), which has no matching
312+
# mechanism; porting it to torch/MLX is tracked as issue #312.
313+
"writestep": "on-disk checkpoint write cadence; NumPy backend only (issue #312)",
314+
"do_history": "periodic weight-history dump; NumPy backend only (issue #312)",
315+
"histstep": "weight-history dump cadence; NumPy backend only (issue #312)",
306316
# Misc.
307317
"decwindow": "decrease-count window size; pamica exposes only max_decs/maxdecs",
308318
"fix_init": "fixed initialization scheme, not ported",

0 commit comments

Comments
 (0)