Address JOSS reviewer feedback on paper and Figure 1#138
Merged
Conversation
Clarify data provenance, iteration counts, LL-gap units, and replace overclaiming "bit-exact" language per reviewer comments. Fix Table 1 to report actual mean-difference distances alongside the ensemble equivalence p-values. Regenerate Figure 1 at its true print size (was drawn ~3x larger than its embed width, collapsing every font to ~3pt) and fix the legend/stats-box collision in Panel A plus a stats-box/bar overlap in Panel B. Add a parameter-file note to the validation docs.
Keeps the committed paper.pdf in sync with paper.md/paper.bib without a manual artifact download each time. Skipped on pull_request runs, since that checkout is a detached merge ref rather than the PR's head branch.
They previously sat inside the plot (in whatever corner looked emptiest) and ended up overlapping the bars and each other no matter where anchored, since three overlapping distributions leave no pocket big enough to hold them. Reserve a fixed bottom margin instead and place both using each panel's actual get_position() center, so nothing sits on top of the data and the two panels' annotations don't bleed into each other. Also states plainly in paper.md/docs that validation currently uses one exemplar recording plus one benchmark subject, with a multi-subject/multi-dataset validation flagged as planned future work rather than implied as already comprehensive.
The out-of-axes placement (previous commit) fixed the overlap but left a lot of dead space; shrink the figure and the gaps between xlabel/legend/stats so the footer reads as one tight block per panel instead of three loosely stacked pieces.
sample_params.json had do_approx_sphere=false, but AMICATorchNG's own default is true and its source comment says false "breaks activation parity" -- confirmed firsthand (0.49 -> 0.99 correlation after the fix). Git blame traces this to an exploratory Jan 2025 commit that was never reverted; the existing regression test never caught it because it builds AMICATorchNG from hand-picked kwargs that never touch this flag. Added a new regression test that exercises the real JSON config so this can't silently recur. Also: fix the bundled input.param's stale max_iter (200, not the documented 2000), reconcile a harmless but confusing min_grad_norm mismatch between the two config files, remove dead/misleadingly- named create_fortran_params, drop two unused ty:ignore comments blocking the pre-commit hook, and tighten a few paper.md claims (Figure 1B's actual axis span, an overgeneralized cross-machine performance claim in Table 2, a run-on sentence, and the config- file-sharing gap flagged in prior review).
Left at the template's literal 32 regardless of channel count, so the 70-channel run silently got PCA-reduced to 32 components and Fortran's W came back the wrong shape. AMICATorchNG has no PCA source reduction, so pcakeep must always track nw.
…MICA into paper-reviewer-response
Fortran (CPU) and NG (GPU) don't compete for the same resource, so multiple seeds' Fortran runs can go concurrently while a prior seed's NG/CUDA phase is still computing, instead of strictly serializing both stages per seed.
Fortran's input.param ships with use_min_dll/use_grad_norm early- stopping ON, so it can converge and halt well short of max_iter while AMICATorchNG (no early-stopping equivalent) always runs the full budget. On the small bundled sample data this rarely bit, but on a larger/higher-k recording it meant NG kept "polishing" past where Fortran had already stopped, letting weakly-determined components drift to a different, still-valid local optimum -- producing lower Fortran-vs-NG correlation at 2000 iterations than at 200-300, which looked like noise but was actually an asymmetric stopping criterion. Disabling both (matching the existing benchmark_dimsweep.py convention for fixed-length, matched runs) makes both implementations run the identical, literal iteration count.
Lets us directly test whether disabling Newton (matching the project's existing benchmark_dimsweep.py convention) resolves the Fortran-vs-NG divergence found at the full 2000-iteration budget, isolating whether the issue is Newton-specific (issue #145).
Fortran (CPU) phases run sequentially at full 24 threads; each seed's NG (GPU) phase launches in the background as soon as its Fortran finishes, overlapping with the next seed's Fortran run since CPU and GPU don't compete for the same resource.
The default do_newton=1 setting shows a much larger Fortran gap at a full 2000-iteration budget (mean corr ~0.97, min ~0.55, issue #145) than the correlation figure previously reported. With Newton off, a 5-seed sweep on both the bundled sample and external 70ch EEG matches Fortran's own run-to-run self-consistency (~0.998), so Table 1 now reports the do_newton=0 result, which is reproducible end to end.
Commit the actual script that produces Table 1's do_newton=0 single-model numbers (previously only run ad hoc, not reproducible from the documented commands) and reference it from the validation guide. Also fix a self-contradictory float32-vs-float64 precision claim (paper said both "four to five" and "seven" significant digits for the same comparison; the measured figure is four to five, matching the project's own prior findings), scope the bit-exactness claim to the non-default density families it actually covers, and correct the multi-model pairwise comparison count (190 within-implementation, 400 cross-implementation, not a blanket 190).
…MICA into paper-reviewer-response
The bundled 32-channel sample (k~30) sits at the project's own data-adequacy boundary, so its 0.998 correlation wasn't fully conclusive evidence on its own. Table 1's single-model headline (correlation, LL gap) now uses the external ds002718 recording at k~153, well past the ~60 threshold where cross-backend agreement plateaus; the bundled sample still backs the Amari distance and the dataset-independent score-function/multi-model checks, so the "no external download" claim is now scoped to just those. Also soften "equivalence" to "similarity" throughout for the multi-model claim: a one-sided permutation test showing no evidence of a gap does not establish formal statistical equivalence. Propagated the same k-factor and wording to docs/guides/validation.md and README.md, and corrected an overstated reproducibility claim about the ds002718 sweep script (it is workstation-specific, not yet a portable one-command reproduction).
…MICA into paper-reviewer-response # Conflicts: # paper.pdf
neuromechanist
marked this pull request as ready for review
July 15, 2026 23:08
CI's base env (no extras) can't resolve the optional mne import in benchmark_decompose.py, and the ignore comments that suppressed this were removed earlier as apparently-unused, since ty only flags them that way in a local env that happens to have the viz extra installed. Restore the ignore comments and disable the unused-ignore-comment rule project-wide, since whether the comment is "used" depends on the developer's own environment, not the code. Verified against both a real local env (mne installed) and a clean CI-equivalent venv (mne absent).
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #138 +/- ##
==========================================
- Coverage 80.20% 80.11% -0.10%
==========================================
Files 14 14
Lines 2102 2102
Branches 357 357
==========================================
- Hits 1686 1684 -2
- Misses 302 303 +1
- Partials 114 115 +1 🚀 New features to boost your workflow:
|
test_end_to_end_correlation_vs_fortran_from_sample_params_json was missing the @pytest.mark.slow and skipif markers its sibling test has, so CI's "pytest -m not slow" run didn't exclude it and it tried to exec the macOS-only amica15mac binary on the Linux runner (Errno 8: Exec format error). Verified it now deselects under -m "not slow" and still passes when run directly.
Address findings from the silent-failure-hunter PR review: - run_ng_only.py, test_ds002718_32ch.py, test_ds002718_hallu.py drove AMICATorchNG directly instead of through the AMICA wrapper, bypassing the project's own degenerate-fit guard (issue #50): a singular_ll stop leaves a finite-but-meaningless W with no error, which would have flowed straight into a reported correlation number. Now checks stop_reason against _DEGENERATE_STOP_REASONS and skips the seed. - test_ds002718_32ch.py and test_ds002718_hallu.py could exit 0 with zero successful seeds (a per-seed Fortran failure just continue'd), indistinguishable from a legitimate empty run. Now track failures and exit 1 if any seed failed or the run produced no results. - Four scripts silently fell back to a NaN/None log-likelihood when Fortran's stdout/out.txt didn't contain a parseable LL line, masking output-format drift. Now print an explicit warning first. - test_ds002718_32ch.py was missing the pcakeep override its sibling script already documents (AMICATorchNG has no PCA reduction, so a stale template pcakeep silently PCA-reduces the Fortran side only). - amari_index in bundled_sample_newton0.py now raises on an all-zero row/column (degenerate W) instead of silently returning NaN, matching validate_implementations.py's existing _amari_index guard. - Fixed a stale issue-140 path in test_ds002718_32ch.py's docstring and a dead duplicate computation in test_ds002718_hallu.py's write_fdt. bundled_sample_newton0.py already routes through the AMICA() wrapper, which calls _check_usable() and was already protected.
Address findings from the comment-analyzer PR review: - Figure 1's embedded stats box labeled all three correlation lines "(190 run pairs)", but the between-implementation line is actually 400 pairs (20x20, not C(20,2)); this contradicted Table 1's own caption two pages later in the compiled PDF. Now labels each line with its actual n, derived from the data rather than hardcoded, and regenerated the figure + rebuilt paper.pdf. - bundled_sample_newton0.py's docstring quoted paper.md's "no external download" claim for Table 1, which was true when written but stale after Table 1's single-model headline moved to the external ds002718 recording; updated to describe what the script actually backs (the bundled-sample Amari row). - Added missing optional trailing CLI args (do_newton, seed_offset) to three scripts' usage docstrings, and --from-cache to multimodel_ensemble.py's. - test_ds002718_32ch.py's binary claim was imprecise: Table 1's headline actually uses the Linux amica15_linux build, not the macOS amica15mac binary this script uses (which does back the bundled Amari row). - Generalized the [tool.ty.rules] comment to name both optional-extra imports it covers (mne, mlx), not just mne.
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
2 participants
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.
Summary
Responds to reviewer feedback from Arnaud Delorme (and a request from Scott
Makeig, tracked separately) on the JOSS paper draft.
30504 samples @ 128Hz) distinctly from the external Wakeman-Henson (ds002718)
benchmark data, states the 2000-iteration/default-parameter run configuration,
and states explicitly that pyAMICA is a full reimplementation, not a Fortran
wrapper.
paper.md(the claim is ~1e-15, not literally 0).absolute difference) and the 0.995 cross-backend agreement figure (a mean).
correlation, -0.011 Amari) alongside the ensemble-equivalence permutation
p-values, plus a footnote on what's averaged. Numbers are recomputed from the
already-saved ensemble data (
.context/issue-27/ensemble.npz), not invented.docs/assets/figures/multimodel-ensemble.png): fixes a real legendvs. stats-box collision in Panel A that a co-author reported, AND a deeper
print-scale bug an independent figure-qa review caught -- the figure was
authored at a 12.5in on-screen canvas but embeds at ~4-5.4in in the actual
compiled PDF, collapsing every font to an unreadable ~3pt in print even though
it looked fine on a monitor. Redrew it at its true print footprint
(figsize matched to the embed width) with print-appropriate font sizes, and
fixed a second stats-box/bar overlap in Panel B found in the same pass.
docs/guides/validation.md(accurate: JSONparam keys mostly, not entirely, match Fortran's
.paramnames)..context/paper-applications-draft.mdfor a future, longer paper -- out ofscope for the 1750-word JOSS draft.
draft-pdf.ymlnow auto-commits the rebuiltpaper.pdfback to thebranch on a direct push (skipped on
pull_requestruns, whose checkout is adetached merge ref rather than the PR's head branch).
Related, filed separately (not in this PR): #132 (native Fortran
.paramreader) and #133-137 (MIR/PMI port epic).
Test plan
Draft PDFworkflow compiledpaper.mdand auto-committedpaper.pdf; pulled it locally and rendered the actual PDF pages at150/300dpi to confirm Figure 1 is fully legible at true print size (no
more legend/stats-box collisions, no overflow) -- verified against the
real compiled output, not just the source figsize math.
.context/issue-27/multimodel_ensemble.pyregenerates the figure fromthe already-cached ensemble (
--from-cache), no re-fitting.ruff check/ruff format/tyall pass (pre-commit).