fix!: consolidated 0.6.0 fixes (supersedes scattered PRs) - #483
Open
ejolly wants to merge 38 commits into
Open
Conversation
…iency
BREAKING: `BrainData.fit()` no longer accepts `design_clean`,
`design_clean_thresh`, `design_clean_exclude_confounds`, or
`design_clean_fill_na`, and no longer runs `DesignMatrix.clean()` implicitly.
It estimates exactly the design it is given. Callers who want columns dropped
call `DesignMatrix.clean()` explicitly. `fit()` now emits a UserWarning when
the design matrix is rank deficient.
fit(model='glm') defaulted to running DesignMatrix.clean() on X, dropping any
column correlating >= 0.95 with an earlier one. Three problems:
1. The criterion is a correlation heuristic, not a rank test, so it dropped
columns from designs that were perfectly estimable. A real dartbrains
first-level design lost poly_1 and poly_2 at rank 48 of 48 — full rank.
2. It is order-dependent. clean() keeps the first of a correlated pair, so
which regressor survives depends on the order the design was built in:
add_dct_basis().add_poly() -> drops poly_1, poly_2
add_poly().add_dct_basis() -> drops cosine_1, cosine_2
Same regressors, same data, two different fitted models.
3. It was silent. The only reporting was behind progress_bar, which fit()
leaves as None, so the estimated model differed from the specified one with
no indication. Downstream this surfaces as a confusing
"Contrast vector length (48) must match number of regressors (46)".
Dropping regressors changes the model, so it is the caller's decision.
DesignMatrix.clean() already exists as the explicit, discoverable way to do it.
Meanwhile the genuinely dangerous case passed silently in both modes: with
cleaning disabled, a singular design fits via pseudo-inverse and splits the
effect evenly across dependent columns, returning finite betas that are not
uniquely determined. fit() now checks the rank and warns with the rank, the
column count, the column names, and the suggested next step (.vif() / .clean()).
It warns rather than raises because over-parameterized designs can still have
estimable contrasts.
Tests written red-first: fit keeps every column of a full-rank design even at
r=0.99; the removed kwargs raise TypeError; a singular design warns with an
actionable message; a well-formed design does not warn.
API docs not regenerated here — docs/api is already drifted from master and
`poe docs-generate` is a separate step.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YVDnsKutg6Yqv99dYmUeuP
…letion
The warning previously recommended `DesignMatrix.clean()` as the fix, which
recommends the weaker tool. Dropping a column does not make its variance
disappear — it reassigns it to whichever correlated column survived, and which
one survives depends on the order the design was built in. Ridge has a unique
solution even when X'X is singular, since (X'X + alpha*I) is always invertible,
and that solution is invariant to column order.
Measured on a design with two regressors at r=0.99:
ridge swap the collinear columns -> weights swap exactly
(equal to float32 solver precision, ~1e-6)
clean() order [a, b, c] keeps ['a', 'c']
order [b, a, c] keeps ['b', 'c'] <- different model
The warning now leads with `fit(model='ridge')`, suggests inspecting with
`.vif()` first, and still mentions `.clean()` as an option while naming its
order-dependence.
Adds test_ridge_is_order_invariant_where_clean_is_not, which pins the property
the recommendation rests on, and extends the warning-content test to assert the
guidance names ridge and vif so it can't silently regress.
The migration guide gains a "Prefer regularization to dropping columns" section
with the worked contrast, plus the caveat that regularization fixes the
estimation problem and not the identifiability one: for exactly collinear
regressors no method separates their individual contributions, so ridge buys a
stable answer rather than a recovered one.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YVDnsKutg6Yqv99dYmUeuP
`find_spikes()` runs two independent detectors — one on the per-TR global
signal, one on the mean absolute frame-to-frame difference — and turned every
detection into its own one-hot indicator column. A single bad volume is
routinely caught by both, so the same TR could be flagged twice, producing
exactly identical regressors:
find_spikes(img, global_spike_cutoff=0.8, diff_spike_cutoff=0.8)
# 23 columns, rank 16 <- rank deficient
That is nltools manufacturing the very degeneracy `BrainData.fit()` now warns
about, from a function whose whole job is to hand you nuisance regressors.
Reproduced on real localizer data: at looser cutoffs, 22 of 23 spike columns
collided with another.
Adds `clean: bool = True` to `nltools.stats.find_spikes`, plumbed through
`BrainData.find_spikes` and `find_spikes_data`. When a TR is flagged by more
than one detector, the duplicates are dropped and the `global_spike` column is
kept, so the tie-break is deterministic rather than insertion-ordered.
`clean=False` restores one column per detection.
This is deduplication of the function's own output, not a modeling decision:
the dropped columns are bitwise identical to ones that remain, so there is no
information loss and no arbitrary choice. That is why it defaults on, unlike
the implicit design cleaning removed from fit() earlier in this PR.
Tests cover: no duplicates by default, full rank by default, clean=False
preserving every detection, global_spike winning the tie-break, distinct TRs
never merged, and confound marking surviving the dedup.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YVDnsKutg6Yqv99dYmUeuP
…ount
Finding no spikes is a normal outcome, but it broke the first-level build for
any clean subject:
spikes = bold.find_spikes(...) # no spikes detected
task.append(spikes, axis=1)
ValueError: All Design Matrices must have the same number of rows!
Polars derives height from columns, so a frame with no columns always reports
0 rows. find_spikes built its empty result as
`pl.DataFrame({"_no_spikes": [0]*n}).drop("_no_spikes")` — dropping the only
column discards the height — and the resulting (0, 0) matrix then failed
append()'s row check against the rest of the design.
A design matrix with no regressors still describes a specific number of
timepoints, so DesignMatrix now takes an optional `n_rows` used when the frame
has no columns; `shape` and `__len__` consult it. find_spikes passes the TR
count through.
`append()` additionally drops regressor-less matrices before the row check, so
this composes even for a column-less matrix built without an explicit height,
and appending only empty matrices returns a copy rather than raising.
This also fixes TestFindSpikes::test_find_spikes_brain_data, which fails on
origin/master for exactly this reason.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YVDnsKutg6Yqv99dYmUeuP
…mily
The inference functions printed a tqdm progress bar unconditionally, with no
way to turn it off. Calling them in a loop -- a calibration study running 100
permutation tests, a per-parcel sweep, anything scripted -- emitted one
progress bar per call, and the only workaround was wrapping every call in
contextlib.redirect_stderr.
Adds `progress_bar: bool = False` across the family, matching the canonical
kwarg table in CLAUDE.md and the existing convention in BrainCollection,
LocalAlignment, and the ridge solvers.
Two shared helpers in algorithms/inference/utils.py replace fourteen
hand-rolled tqdm sites:
maybe_tqdm(iterable, *, progress_bar, **kw) # iteration-driven bars
make_progress_bar(*, progress_bar, **kw) # manually-updated bars
`make_progress_bar` returns a `_NullProgressBar` when disabled, so call sites
that drive a bar via .update() need no branching. Both import tqdm lazily, so
it stays off the import path when unused.
Threaded through both layers, since nltools.stats re-exports thin wrappers that
are distinct function objects from the algorithm-layer functions:
algorithms/inference: one_sample, two_sample, correlation, matrix,
timeseries, bootstrap (5 private helpers), isc
nltools.stats: the five permutation wrappers
facades: Adjacency.similarity / .ttest, stats_label_distance
BREAKING: progress bars are now off by default. isc_permutation_test and
isc_group_permutation_test previously defaulted to progress_bar=True and now
default to False; every other function in the family previously had no way to
disable its bar. Pass progress_bar=True to restore the old output.
Tests cover both layers: that the kwarg exists, that it defaults to False, that
nothing reaches stderr by default, and that progress_bar=True still produces a
bar (so the knob is a real toggle, not a silent no-op).
The public inference functions accepted every option positionally -- isc_permutation_test had 15 positional-or-keyword params and 0 keyword-only -- and dispatched into private helpers with 8-11 positional arguments. Inserting a parameter anywhere in one of those signatures silently shifts every argument after it. That is not hypothetical. While adding progress_bar in the previous commit, a parameter inserted ahead of `single_feature` shifted its value into `progress_bar` at the dispatch site. No error, no type failure -- the bar simply came back on, and only an assertion that stderr stays empty caught it. Changes: - `*` marker after the leading data arguments on the seven public entry points (one_sample, two_sample, correlation, matrix, timeseries, isc, isc_group), as CLAUDE.md requires for any public function with 3+ kwargs. - private CPU/GPU helpers made keyword-only too, so the dispatch sites cannot be positional even internally. - all seven dispatch call sites converted to keyword arguments. - scripts/check_kwonly.py extended to cover nltools/algorithms/inference. It excluded the whole algorithms package as "internals", but this module is documented in docs/api/algorithms/inference.md and imported directly downstream, so the facade-translation rationale does not apply. Verified the expanded scope actually fires by removing a `*` and confirming it fails. Fixes a latent bug this exposed: the two_sample GPU dispatch never forwarded progress_bar at all, so progress_bar=True was silently ignored on that path. Also adds an interim signature-parity test asserting the nltools.stats wrappers and the engines agree on every parameter and default except the documented parallel/backend -> device rename. That test would have caught the drift that motivated all of this. The underlying question -- whether the two layers should export 13 same-named distinct objects at all -- is design discussion #474. BREAKING: options must now be passed by keyword. `one_sample_permutation_test(data, 5000)` becomes `one_sample_permutation_test(data, n_permute=5000)`. The leading data arguments remain positional.
…p clean= The colliding detections from the two detectors are bitwise-identical one-hot columns, so deduplication only decides which NAME survives (global_spike, deterministically) — there is no information to preserve and therefore no reason for an opt-out. An escape hatch would only manufacture straight duplicate columns, which append(axis=1) now refuses. Tests discover detector collisions by running each detector solo instead of relying on the removed clean=False path.
Appending a column whose values are identical to an existing column (under any name) now raises ValueError, just as duplicate names already did. A design with straight duplicate columns is rank deficient by construction — the model over it is not computable — and silently keeping one copy would be a modeling decision made on the user's behalf. Columns are compared on Float64 byte representation, so an int one-hot and its float twin count as the same regressor. Only duplication introduced by the append is checked; a base matrix that already contains duplicates is left to its owner. BREAKING: DesignMatrix.append(axis=1) raises ValueError when an appended column's values are bitwise identical to another column's, where it previously appended silently. Drop or modify one of the columns first.
…values n_rows (the recorded length of a column-less DesignMatrix, new in this branch) was dropped by every copy path — copy()/copy_with()/the copy-constructor silently reset a (60, 0) matrix to (0, 0) — and a value that contradicted a non-empty frame was silently ignored. Now: get_metadata/copy_with carry n_rows (falling back to the source's height when a transform empties the frame), the copy-constructor inherits it, a conflicting or negative n_rows raises ValueError, and to_numpy()/np.asarray() honor the recorded length by returning an (n, 0) array instead of (0, 0).
…clean' into 0.6.0-fixes # Conflicts: # docs/migration-guide.md
…onable Reworks the warning that replaced fit()'s implicit design cleaning (merged from refactor/remove-implicit-design-clean), per maintainer feedback on the RFC: a loud warning with helpful tips rather than a policy lecture. - Names the likely-involved columns via pivoted QR (truncated at 5) instead of dumping every column name into the message. - Catches p > n designs — rank deficient by construction — which the old guard silently skipped (the one case guaranteed to be degenerate got no warning at all). - Offers all three next steps: inspect with .vif(), drop redundant columns with .clean() (with the order-dependence caveat), or fit(model='ridge') for a unique, order-invariant solution. - Dedicated RankDeficientDesignWarning category (UserWarning subclass) so it stands out in output and can be silenced surgically. Unit tests cover the pure helper directly (fast); the slow fit() integration tests continue to pass unchanged. Migration guide quote synced.
Collaborator
Author
|
Landed: the #470 rework (no implicit design cleaning + a diagnostic rank-deficiency warning). Supersedes #470. The RFC branch is merged in as-is (all four
Sample output: Decision resolved: warn (never raise, never auto-clean) — dropping regressors is a modeling decision that belongs to the caller; the warning's job is to make the degeneracy impossible to miss and the options obvious. Known follow-up (out of scope here): warnings raised inside |
This was referenced Aug 20, 2026
…-wide progress mechanism Promotes the two helpers added by the inference progress_bar work from nltools/algorithms/inference/utils.py to nltools/utils.py and converts every hand-rolled tqdm site to use them: alignment/local.py (x2), ridge/solvers.py, braindata/neighborhoods.py, braindata/prediction.py (x2), collection/execution.py (tqdm_joblib), and the five isc.py loops. The helpers now use tqdm.auto, so notebooks get widget bars everywhere (previously only collection ops did). A source-scan test pins the invariant: no module outside nltools/utils.py may import tqdm. Also answers the review ask to measure bar overhead before standardizing: at bench_inference sizes (30x5000, n_permute=1000, CPU) bars cost +1.6% (one_sample), +4.1% (two_sample), +12.7% (correlation — 7ms on a 60ms workload), confirming off-by-default. With bars off by default the benchmark's _quiet() stderr redirect is dead code and is removed. Dropped the try/except ImportError fallbacks around tqdm — it is a hard core dependency. The searchlight decode path previously materialized list(tqdm(...)) up front, driving the bar to 100% before any decoding ran; bars now advance during the actual work.
…at facades The inference engines' progress_bar kwarg was unreachable from four public facades that previously showed bars unconditionally — Adjacency.similarity, Adjacency.ttest, Adjacency.bootstrap, and BrainData.bootstrap silently went from always-on to always-off with no knob. Each now exposes progress_bar: bool = False (keyword-only, canonical trailing position) and forwards it down the delegation chain (adjacency/modeling.py bootstrap, braindata/bootstrap.py and its five engine call sites). Also removes a phantom progress_bar entry from phase_randomize's docstring (the function has no such kwarg — it has no permutation loop), documents the kwarg in the module-level similarity() docstring, and adds the missing migration-guide entry for the one silent behavior change in the family: isc_permutation_test / isc_group_permutation_test flipped from progress_bar=True to False.
Collaborator
Author
|
Landed: the #473 rework (inference The
Coming next on this branch: the #475 rework (keyword-only enforced everywhere, with a documented exemption for the numpy-mirroring |
…s the package Extends the #475 work per review: check_kwonly.py now scans all of nltools/ (tests excluded) instead of a curated root list with an algorithms/ carve-out, and the 12 remaining violations are fixed — SRM/DetSRM.__init__, ridge_svd/ridge_cv, KFoldStratified.__init__ (matching sklearn's own KFold(n_splits, *, ...) shape), plot_mean_label_distance, plot_between_label_distance, and plot_interactive_brain all gain the `*` marker. Every internal call site already passed these options as keywords, so no in-repo behavior changes; external positional callers now get a TypeError. The four numpy-mirroring shims in algorithms/backends.py (zeros_like, ones_like, full_like, assert_array_almost_equal) are exempted via an explicit, documented EXEMPT list in the checker — their signatures deliberately match numpy for drop-in substitutability, and a `*` would break that. The list is for externally-dictated signatures only. Also finishes the private-dispatch hardening the original PR described but missed: `*` added to _matrix_permutation_cpu_parallel and the five private isc.py helpers, so inference dispatch cannot go positional even internally. BREAKING: options on the newly-marked public functions must now be passed by keyword (e.g. KFoldStratified(5, True) -> KFoldStratified(5, shuffle=True)). Gates: poe lint, poe lint-api (semgrep + check_kwonly + vocabulary), and the full default suite (1755 passed).
Collaborator
Author
|
Landed: the #475 rework (keyword-only enforced everywhere). Supersedes #475. The
Gates: CI runs Both inference PRs (#473, #475) are now fully absorbed. Coming next: fixes for #478 / #474 / #479 (per the sequencing note on #474, the |
This was referenced Aug 20, 2026
…al core into nltools.algorithms Closes the full-consolidation scope of #474. nltools.stats had become a compatibility layer over the functional core; v0.6.0 makes nltools.algorithms the single entry point — every user-facing statistical function is importable flat from it (51 exports), organized into focused submodules underneath. BREAKING: nltools.stats is removed. Implementations moved: corrections.py, outliers.py, regression.py as-is; timeseries.py -> algorithms/signal.py and correlation.py -> algorithms/similarity.py (avoids name echoes with the inference engine's modules); intersubject.py -> algorithms/inference/; alignment.py -> algorithms/alignment/procrustes.py. stats/permutation.py is deleted outright — the nltools.algorithms permutation exports ARE the algorithms.inference engine functions (identity, no wrapper layer), so facade/engine drift is structurally impossible. BREAKING: the inference engine speaks the canonical device= vocabulary directly. parallel= -> device= on every algorithms.inference entry point (values unchanged: 'cpu' | 'gpu' | None), validate_parallel_parameter -> validate_device_parameter, and result dicts report a 'device' key instead of 'parallel'. phase_randomize(backend='numpy'|'torch') is now keyword-only device='cpu'|'gpu'|'auto' with warn-and-fallback on unknown values. The ridge and alignment layers keep parallel= internally (documented Backend abstraction); facades still translate at those boundaries. Details: - The intersubject functions (isc/isc_group/isfc/isps) export flat from nltools.algorithms only, imported straight from inference/intersubject.py — re-exporting the isc *function* from the inference package would shadow the inference.isc engine *module*. - Tests: red-first test_algorithms_api.py (flat surface, stats gone, wrapper-free identity) and test_inference/test_device_kwarg.py; tests/stats/ -> tests/core/test_algorithms/; the facade-parity suites are superseded by the identity tests and removed. - Semgrep vocabulary rules now enforce algorithms/inference (exclusions narrowed to ridge/ + alignment/ + backends.py); poe test-stats -> test-algorithms. - Docs kept in sync: migration guide gains the (stats-module-removed) section with the full old->new mapping and all stale nltools.stats/parallel= guidance rewritten; build_api_docs.py + myst.yml map the new modules; CLAUDE.md, development/index.md, inference-internals.md, api-vocabulary.yml (+ regenerated AUTOGEN blocks), and the GLM tutorial updated. Site regeneration stays batched for the deferred docs-generate pass. Gates: 1769 passed (default suite), poe lint, poe lint-api.
…c=/null_dist) Follow-up to the #474 consolidation: the ISC family now speaks the canonical kwarg table end to end, removing the last boundary translation the old nltools.stats wrapper performed. BREAKING: isc_permutation_test / isc_group_permutation_test rename metric= (the 'median'|'mean' central-tendency choice) to summary=, and sim_metric= (the similarity metric) to metric= — `metric` now means the same thing here as everywhere else in the API. isc_group() and BrainCollection.isc/.isc_test likewise take summary= instead of metric= (isc() already used summary=). BREAKING: every ISC result — engines, the isc/isc_group wrappers, and BrainCollection — exposes the null under the engine-standard 'null_dist' key; the legacy 'null_distribution' key is removed. Also: the isc/isc_group wrappers expose progress_bar: bool = False and forward it (previously hard-coded off), completing the progress-bar threading; wrapper docstrings and the collection helpers (_aggregate_corrs and the streaming ISC paths) renamed to match. Tests: red-first test_inference/test_isc_vocabulary.py (14 tests: canonical signatures on all six entry points incl. BrainCollection, null_dist keys, summary validation, wrapper progress-bar threading); existing isc suites updated to the new vocabulary. Migration guide and execution-model.md updated in place. Gates: 1783 passed (default suite), poe lint, poe lint-api.
Collaborator
Author
Landed: #474 full consolidation —
|
BREAKING: Adjacency.cluster_summary renames method= -> summary=
('mean'|'median'|None central tendency) and its old summary= (within/between
scope) -> scope=. BrainData.extract_roi renames metric= -> method=
('mean'|'median'|'pca' selects an extraction variant; metric stays reserved
for similarity metrics). Closes the last two mean/median-vocabulary
violations flagged in the #474 consolidation follow-up.
…vocabulary.yml docs/_data/api-vocabulary.yml is now the single source of truth for the API vocabulary: it gained an `enforcement:` section (banned alias kwargs, per-kwarg semantic contracts, structured exceptions/exemptions) and a new AST checker (scripts/check_api_vocabulary.py, first step of `poe lint-api`) validates every public signature in the package against it. The six banned-kwarg semgrep rules migrated into the manifest so banned lists live in exactly one place; semgrep keeps the result-key and **kwargs rules. CLAUDE.md and the docs now point at the manifest instead of duplicating the table (which moved its three CLAUDE.md-only rows — threshold pair, include_diag, radius_mm — into the YAML). The checker surfaced and this commit fixes three drift sites: BREAKING: BrainData.fit defaults progress_bar=False and no longer inherits bd.verbose when unset (verbose is reserved for log-level only). SphereNeighborhoods.iter_neighborhoods takes progress_bar keyword-only. Deliberate deviations are now recorded as manifest exemptions with reasons (BrainData.predict n_jobs=1 memory guard; align n_iter solver iterations).
Collaborator
Author
Landed: cluster_summary/extract_roi canonicalization + programmatic vocabulary enforcement (scope expansion)Two more chunks on
|
…#474) Every sign-ambiguous p-value now defaults two-tailed and speaks one vocabulary: tail: int | str = 2, accepting 2|'two' (default) and 1|'one' (one-tailed in the test's canonical positive direction — never chosen from the data). Default (tail=2) output is numerically unchanged everywhere. Enforced via a tail contract in api-vocabulary.yml. BREAKING: the v0.5 -1/'upper'/'lower' public forms now raise ValueError (negate the data / swap groups / flip the contrast for the negative direction; _compute_pvalue keeps them internally for forced-tail sites). BrainData.ttest and Adjacency.ttest previously ignored tail= on the parametric path (always two-sided); tail now maps onto scipy's alternative=, and the z map matches the requested tail. New tail= options (default 2 ≡ old behavior): BrainData.ttest2, BrainData/Adjacency.bootstrap, multivariate_similarity, regress, Adjacency.regress, BrainCollection.ttest/.ttest2/.isc_test, Roc.calculate. Collection permutation tests accept the string forms and route through the shared engine p-value. Sites where only one tail is statistically valid (dcorr, ANOVA F, isps Rayleigh, SRM variance) are untouched, and GLM contrast p-maps stay nilearn one-sided as a documented exception.
Collaborator
Author
Landed: canonical
|
Collaborator
Author
Remaining work before/after this PR merges
Everything this PR set out to consolidate has landed (supersedes #470, #472, #473, #475, #477 — all closed/merged — and completes #474). What's left, in rough priority order: On this branch, before merge
Scoped and ready to implement (this PR or fast-follows)
Newly filed
The vocabulary/enforcement work from this PR (api-vocabulary.yml manifest + |
Every column nltools invents rather than the user now carries the reserved prefix `.nl_`: `.nl_poly_0` / `.nl_cosine_1` (add_poly / add_dct_basis), `.nl_global_spike1` / `.nl_diff_spike1` (find_spikes), and the run-separated variants a multi-run append produces (`0_poly_0` -> `.nl_r0_poly_0`; the run index moved inside the prefix and prefixes never stack). The point is detection. Machinery that has to recognize nltools' own columns was pattern-matching user-controlled names and was wrong in both directions: - add_poly() counted underscores, so a single-run design carrying the standard 24-parameter motion expansion (trans_x_sq, rot_x_diff_sq, ...) could not have drift terms added at all -- it raised about run-separated polynomials that were never there (#471). add_dct_basis() carried the same flawed guard. Both now share one predicate keyed on the reserved namespace, and both refuse per-run polynomial *and* cosine drift (previously each saw only its own kind). - vif(exclude_confounds=False) dropped any column whose name merely contained "poly_0" while missing the all-ones `cosine_0` it actually needed to drop. Now keyed on a generated-intercept predicate, so the DCT constant is excluded and user columns never are. nltools/utils.py holds the single source of truth: RESERVED_PREFIX plus reserved_name / run_separated_name to build names and is_reserved_name / parse_run_separated to recognize them. append(axis=1) refuses a raw pandas/polars frame whose columns use the prefix -- those columns are the user's by definition, and admitting them would defeat the namespace. Users may now name their own regressors anything, including `poly_0`, without colliding with the machinery. BREAKING: all generated DesignMatrix / find_spikes column names gained the `.nl_` prefix, and run-separated columns changed shape from `{run}_{col}` to `.nl_r{run}_{col}`. Code selecting generated columns by name must be updated; see the (reserved-column-prefix) section of the migration guide. Gates: 1873 passed, `poe lint` and full `poe lint-api` green.
Writing a DesignMatrix and reading it back did not work, for two independent
reasons.
**A `.csv` was written tab-separated.** `write()` defaulted to a tab delimiter
whatever the extension, while the file constructor chose the delimiter from the
extension, so `dm.write("design.csv")` read back as a single column named
`'cond_a\tcond_b'`. The delimiter now comes from one helper
(`separator_for_path`) used by both sides: `.csv` means comma, everything else
means tab. An explicit `sep=` still overrides. Files already on disk with a
mismatched delimiter are detected on read -- a single parsed column whose name
contains the other delimiter is an unambiguous tell -- and re-parsed, so they
load without intervention.
**There was no `.h5` reader.** `write_h5` produced a valid file that nothing
could open: the constructor routed every path to the CSV reader, which died
with `ComputeError: invalid utf-8 sequence`. Adds `read_h5` and dispatches on
`is_h5_path`. Because such a file is a serialized DesignMatrix rather than a
table awaiting interpretation, it requires neither `run_length` nor
`sampling_freq`, and it restores `sampling_freq`, `.convolved`, `.confounds`,
`.multi`, and the recorded row count of a column-less matrix (so `find_spikes`
output for a clean subject round-trips as `(n_tr, 0)`). Explicit kwargs still
override what the file recorded.
`write_h5` now stores the frame as Arrow IPC bytes through the existing
`nltools.io.h5` helpers rather than a homogeneous numpy array, so column dtypes
survive exactly -- an integer spike indicator comes back an integer. The
previous layout (float matrix + `S`-typed `columns` dataset) is still read.
Neither path had any test coverage; adds `test_designmatrix_io.py` (16 tests)
covering text symmetry per extension, the mismatched-separator fallback, h5
metadata/dtype/empty-matrix round trips, and the legacy h5 layout.
BREAKING: `write()` to a `.csv` now emits comma-separated data (it emitted tabs
before, which its own reader could not parse). Code parsing nltools-written
`.csv` files with an explicit tab delimiter must switch to comma or pass
`sep="\t"` to `write()`.
Gates: 1889 passed, `poe lint` and full `poe lint-api` green.
Collaborator
Author
Landed: #471 as a reserved column namespace, plus the DesignMatrix file round-tripTwo commits on
|
| v0.5.1 | v0.6.0 | Produced by |
|---|---|---|
poly_0, poly_1, … |
.nl_poly_0, .nl_poly_1, … |
add_poly() |
cosine_0, cosine_1, … |
.nl_cosine_0, .nl_cosine_1, … |
add_dct_basis() |
global_spike1, diff_spike1, … |
.nl_global_spike1, .nl_diff_spike1, … |
find_spikes() |
0_poly_0, 1_motion_x, … |
.nl_r0_poly_0, .nl_r1_motion_x, … |
append(axis=0, keep_separate=True) |
Run separation moved the index inside the prefix and gained an r so it's parseable, and prefixes never stack (.nl_poly_0 in run 1 → .nl_r1_poly_0). It applies to your own confounds too (motion_x → .nl_r0_motion_x): the run-prefixed variant is a name nltools invented, so it belongs to the namespace.
Two detection bugs this kills:
add_poly()/add_dct_basis()(the reported bug): a single-run design carrying the standard 24-parameter motion expansion couldn't add drift terms at all. Both adders now share one predicate keyed on the prefix. Per fix(designmatrix): add_poly() rejects designs with ordinary 2-underscore confounds #471's rework, both also refuse per-run polynomial and cosine drift — that shared-guard shape is kept, just keyed on the namespace rather than a regex over arbitrary names.vif(exclude_confounds=False)(found while auditing for the same pattern, not in fix(designmatrix): add_poly() rejects designs with ordinary 2-underscore confounds #471): it excluded columns via"poly_0" not in c, which dropped a user column namedmy_poly_0_estimatewhile missing the all-onescosine_0it actually needed to drop — leaving a singular correlation matrix. Now keyed on a generated-intercept predicate that covers both.nl_poly_0and.nl_cosine_0plus their run-separated variants.
nltools/utils.py holds the single source of truth: RESERVED_PREFIX plus reserved_name() / run_separated_name() to build names and is_reserved_name() / parse_run_separated() to recognize them. append(axis=1) refuses a raw pandas/polars frame whose columns use the prefix — those columns are the user's by definition, and admitting them would defeat the namespace. DesignMatrix inputs are unaffected.
Net effect for users: you can now name your own regressors anything, including poly_0, without colliding with the machinery.
3aea7d63 — write() and the file constructor now round-trip (breaking)
Found while checking that dotted column names survive a save/load. They didn't — but neither did anything else, for two reasons unrelated to the rename. Neither path had any test coverage, which is why both survived.
- A
.csvwas written tab-separated.write()defaulted to a tab delimiter whatever the extension while the constructor chose the delimiter from the extension, sodm.write("design.csv")read back as one column named'cond_a\tcond_b'. Both sides now derive it from one helper (.csv→ comma, else tab); explicitsep=still overrides. Files already on disk with the mismatched delimiter are detected and re-parsed, so they load without intervention. - There was no
.h5reader.write_h5produced a valid file nothing could open — every path went to the CSV reader, which died withComputeError: invalid utf-8 sequence.DesignMatrix("design.h5")now works and, since an.h5is a serialized object rather than a table awaiting interpretation, needs neitherrun_lengthnorsampling_freq; it restoressampling_freq,.convolved,.confounds,.multi, and the recorded height of a column-less matrix (sofind_spikesoutput for a clean subject round-trips as(n_tr, 0)).
The h5 writer now stores the frame as Arrow IPC bytes through the existing nltools/io/h5.py helpers instead of to_numpy(), so dtypes survive exactly — an integer spike indicator comes back an integer, and a DM with any non-numeric column no longer produces an object array h5py rejects. The previous layout is still read.
BREAKING beyond the renames: write() to a .csv now emits comma-separated data. Anything parsing an nltools-written .csv with an explicit tab delimiter needs to switch.
Verification
1889 passed (default suite), poe lint and full poe lint-api green. Migration guide gained (reserved-column-prefix) and (designmatrix-file-round-trip); the stale "there is no DM HDF5 reader yet" line is now accurate. The naming convention is recorded as a design rule in docs/development/index.md and CLAUDE.md.
Remaining work
On this branch, before merge
- Batched docs regeneration:
poe docs-generate(themyst.ymlTOC references per-modulealgorithmsAPI pages griffe2md hasn't generated, and the committeddocs/api/*.mdstill show pre-rename column names) +poe changelog(git-cliff regen for the!:commits). Smoke-builddocs-siteafter, since the TOC is currently ahead of the generated sources.
Scoped and ready to implement
- iplot(): window opens at raw min/max — add robust autoscaling and percentile thresholds #479 —
iplot()robust autoscaling: compute the window in Python (98th percentile of |nonzero| as ceiling, epsilon floor), percentile threshold strings via a sharedresolve_threshold()innltools/utils.py,autoscale: bool | tuple = True. Fold in thecal_min/slider inconsistency from the issue — once Python owns the window, always pass explicitcal_min/cal_maxso the handles can't show one window while niivue renders another. One decision embedded:threshold()computes its percentile overb.dataincluding zeros, which skews it for a masked map; the viewer wants nonzero. Sharing the helper means deciding whether to fix that in both (a silent change to thresholding results) or keep them deliberately different. - Design: should BrainCollection.predict() aggregate subjects, or map over them? #478 —
BrainCollection.predict()map-reduce: direction is settled (map, per the issue discussion). Phase 1 is thepredict_group()carve-out plus the int-cvgroups=discard fix — small, and could ride this branch if we want the breaking rename inside 0.6.0 rather than a 0.6.1 deprecation. Phase 2 is the real work: per-subjectPredictresults need aPredictCollectioncontainer, which raises whether per-subject prediction routes through the_ItemTask/path-backed execution machinery and what the on-disk form is (the HDF5 fit bundles are the precedent) — that touches the execution model, so it wants its own PR. Phase 3: decide the legacycv()path's fate and wire or remove the inertCVScheme.split_by(0.6.0 is the window).
Needs a scoping call
- Survey remaining gpu support #484 — remaining GPU support: recommendation for 0.6.0 is no new GPU implementations — instead make every advertised-but-deferred site fail loudly and consistently (matching what
matrix_permutation_testand Spearman/Kendall already do), fix the two silent offenders (SRM/DetSRMsilently falls back to CPU;BrainCollection.permutation_test/permutation_test2treatdevice=as informational), and record the rest as the 0.6.x roadmap. Real GPU paths are each their own project and shouldn't gate the release.
Collaborator
Author
|
Scoping #484 is a requirement for 0.6.0 - need to revisit with proper scoping plan |
Collaborator
Author
Current status: remaining-work decisions resolved — final scope for this PRAll open decisions from the previous status comment are now resolved (that comment is superseded by this one). Basis for #484: the site-by-site GPU survey. Decisions
Implementation order
|
…OM recovery, run-or-raise Consolidates every GPU/batched code path onto a single core layer in algorithms/backends.py, closing the #484 requirement for 0.6.0 (scoping survey: #484, decisions: PR #483 thread): - device_memory_budget(): max_gpu_memory_gb=None (the new default everywhere) measures the device — free CUDA memory with headroom, available system RAM for MPS/CPU — instead of assuming 4 GB (or the hard-coded 8 GB inside SRM/hyperalignment worker sizing). - auto_batch_size(): the one batch calculator, replacing five divergent implementations (inference _auto_batch_size, bootstrap _auto_batch_size_ridge, ridge _auto_n_targets_batch, LocalAlignment._auto_batch_size, isc's byte math). Algorithms now supply only their per-item working-set estimate; a source-scan test pins budget math out of every other module. - compute_oom_safe(): reactive OOM recovery in every GPU batched loop — empty cache, split the already-drawn batch inputs, retry. RNG draws stay outside the retried compute, so a seeded result is bit-identical with or without OOM (pinned by test_oom_recovery.py). - auto_n_jobs_for_arrays() + the n_jobs helpers moved from inference/utils (re-exported), deduplicating six copy-pasted worker-sizing blocks in srm/hyperalignment. Run-or-raise policy (api-vocabulary.yml + docs/development/index.md): explicit device='gpu'/parallel='gpu' runs on the GPU or raises; 'auto' is the only graceful fallback. - SRM/DetSRM raise NotImplementedError on parallel='gpu' (was silent CPU fallback); the dead max_gpu_memory_gb kwarg is removed. - LocalAlignment validates parallel= (typos silently ran numpy), raises for gpu+srm/hyperalignment and for gpu without torch. - Kendall GPU is real: tie-corrected tau-b kernel via pre-computed pairwise sign tensors (permutation-invariant denominator), parity- tested against scipy.kendalltau — the warn-and-fallback is gone. - BrainCollection.permutation_test/permutation_test2 delegate to the engine (like isc_test): device=/n_jobs= were informational, now real; both gain progress_bar=; ~60 duplicated lines deleted. Also deletes the stale algorithms/inference/DESIGN.md (superseded by docs/development/inference-internals.md). BREAKING: SRM/DetSRM.fit() no longer accepts max_gpu_memory_gb and parallel='gpu' raises; LocalAlignment rejects unknown parallel= values and gpu-without-torch; max_gpu_memory_gb defaults changed from 4.0 to None (measured) across inference/ridge/braindata entry points; seeded null distributions from BrainCollection.permutation_test/ permutation_test2 change (engine RNG replaces the hand-rolled loop). Migration guide: (gpu-execution-layer).
…pipeline #478 phase 1. BrainCollection.predict(y=...) aggregated — one model with subjects as samples — while every other per-subject method maps. The aggregate operation now lives under an explicit name: - predict_group(y, *, ...) is the old predict(y=) group-MVPA path, returning the same Predict dataclass. predict(y=...) raises with guidance; predict(X_new=) (per-subject predict-after-fit) unchanged. The per-subject decoding form of predict(y=) is #478 phases 2-3. - Bug fix: an int cv= no longer discards groups=. predict(cv=5, groups=subject_ids) resolved to plain KFold, whose groups argument is documented "always ignored" — folds were byte-identical to passing no groups, same subject in train and test. The new nltools.cross_validation.resolve_group_cv resolves int specs to StratifiedGroupKFold (classifiers) / GroupKFold (regressors) when groups are supplied; a behavioral test pins that a group never straddles a fold boundary. - The legacy cv() pipeline is deleted: BrainCollectionPipeline, pipeline.py, the pipesteps/ machinery, and the never-read CVScheme.split_by knob. Its one capability predict_group lacked — the label-permutation null — is ported: predict_group(n_permute=, random_state=) attaches permutation_scores / permutation_pvalue (new fields on Predict), same (1 + #(null >= obs)) / (n + 1) form. BREAKING: BrainCollection.predict(y=...) raises (use predict_group); BrainCollection.cv() and BrainCollectionPipeline are removed. Migration guide: (predict-group).
…tile thresholds Closes #479. iplot() opened with its window at the raw data min/max, so a couple of outlier voxels set the whole color scale — a single-subject beta map rendered as washed-out noise and the 3D view as a solid box (the same bug: cal_min at the data minimum leaves no voxel transparent). - autoscale: bool | tuple = True on iplot(): the default window ceiling is the 98th percentile of the finite nonzero magnitudes (outliers no longer set the scale — nilearn/FSLeyes practice) with an epsilon floor (zeros transparent, everything else visible; threshold up from there). (lo_pct, hi_pct) picks both edges; False is the old extremes window, made explicit. Explicit threshold/lower/upper override per edge. Window logic lives in viewer.compute_display_window (pure function). - nltools.utils.resolve_threshold() is the single source of truth for percentile strings ("98%"), now accepted by iplot's threshold/lower/ upper like threshold() always did. Both resolve over finite NONZERO values — on a masked map most voxels are exactly zero (absence of data) and previously dragged every percentile toward zero. iplot resolves magnitude percentiles (divergent magnitude window); threshold() signed ones. - The slider always shows the rendered window: cal_min/cal_max are always computed in Python and passed explicitly, ending the state where the handles showed the extremes while niivue rendered its own auto window (and the first slider touch destroyed it). BREAKING: threshold(upper="98%") results change on any map containing zeros (percentile now over nonzero voxels); iplot's default window is autoscaled rather than min/max. Migration guide: (iplot-autoscale).
…leanup The deferred docs pass for the 0.6.0 consolidation (PR #483): - poe docs-generate: 65 API pages regenerated, catching the committed docs/api/*.md up with the myst.yml TOC (per-module algorithms pages) and every rename this branch landed (predict_group, the core GPU layer in backends, measured max_gpu_memory_gb defaults, Kendall GPU, .nl_ columns); tutorial .md regenerated from the marimo sources. - poe changelog: git-cliff regen picks up the branch's breaking commits. - Fixed four inference docstrings (one_sample, two_sample, correlation, matrix) still documenting the removed v0.5 tail forms ('upper'/'lower'/-1) that now raise — found during the #484 audit. - 04_brain_collection tutorial prose: bc.predict(y=)/bc.cv() pointer updated to predict_group(). Verified with a full `poe docs-site --execute` smoke build: exit 0, no execution errors.
Collaborator
Author
Landed: the full resolved scope — #484 (GPU layer), #478 phase 1, #479, batched docs passAll four phases from the decision record are on the branch. Supersedes the record's "remaining work" list; nothing is left before merge.
|
Collaborator
Author
Remaining work — open items and pending callsCorrection to the previous status comment: its "nothing is left before merge" line and the "fast-follow" labels were premature — whether each item below lands in this PR or after it is an open call, not a settled one. This is the complete inventory of what remains; none of it is scheduled. Implementation items still open
Verification not yet done on this branch
Calls that are yours to make
|
…ot bitwise CI (torch-cpu, linux) failed test_oom_recovery test_correlation[pearson]: 37/1500 null values differed by <= 6e-08 (one float32 ulp), mismatches starting exactly at the recovery sub-batch boundaries (rows 36, 73). The recovery design holds — the pre-drawn permutations are reused exactly — but torch blocks einsum/std reductions differently per batch shape, so bitwise equality across splits was an overclaim (it happened to hold on MPS locally). - test_oom_recovery: one shared comparison at rtol=1e-4/atol=1e-6 for null distributions and a single-boundary-count-flip tolerance (1.01/(n+1)) for p-values, applied to all three determinism tests so the one_sample/two_sample variants can't flake the same way later. - compute_oom_safe docstring, inference-internals.md, and the migration guide now state the guarantee precisely: identical draws, outputs equal to within float32 reduction order.
…e 'loso'/'loro' resolve_group_cv generalizes to resolve_cv, the single cv-spec resolution rule for every prediction level: 'loo' -> LeaveOneOut, 'logo' -> LeaveOneGroupOut, int -> (Stratified)KFold honoring groups= via the Group variants, splitter passthrough. The removed domain names raise with migration guidance — both were LeaveOneGroupOut differing only in the implied grouping, which groups= expresses directly. predict_group's default becomes cv='logo' (identical splits: groups still defaults to one per subject = leave-one-subject-out); leave-one-run-out is now cv='logo', groups='run'. Int specs for classifiers now resolve to StratifiedKFold (previously plain KFold at the group level). BREAKING: cv='loso'/'loro' removed; use cv='logo' with groups=. predict_group(cv=<int>) with a classifier now stratifies folds. Part of #478 (phases 2-3 groundwork).
…ls travel with the data
BrainData.predict(y=None) now falls back to a single-column .Y frame;
y='name' and groups='name' select columns of a multi-column .Y (the
row-aligned metadata carrier, so within-subject grouping variables like
run live beside the labels). An object with both a fitted encoding model
and a stored .Y refuses the no-argument call as ambiguous. validate_frame
accepts dicts so `bd.Y = {"label": arr, "run": runs}` just works.
predict_mvpa's cv resolution now routes through resolve_cv: int specs
accept and honor groups= (previously silently discarded — the same bug
predict_group had), and cv='loo'/'logo' work at the single-subject level.
BREAKING: predict(cv=<int>, groups=...) now group-aware; previously
groups was ignored for int cv specs.
The per-subject half of #478: BrainCollection.predict(y=...) maps this.
…ntainer Frozen, sequence-like container: one Predict per subject plus the source collection's per-subject metadata. mean_scores/std_scores stack score summaries, scores renders the whole-brain case as a polars table, and weight_maps/accuracy_maps stack the per-subject maps into one BrainData (n_subjects, n_voxels) — the bridge to second-level inference. Part of #478 phases 2-3.
…478 map-reduce gap BrainCollection.predict(y=...) now does what every other method on the class does: one operation per subject. It maps BrainData.predict over the items — one model per subject, CV within that subject's own rows — and returns a PredictCollection carrying the collection's metadata, with weight_maps/accuracy_maps stacking for second-level inference. Label resolution mirrors the class's data model: y=None decodes each subject's single-column .Y, y='name' picks a .Y column, one shared array applies everywhere, and a list of arrays is per-subject. groups= resolves the same way for within-subject schemes (cv='logo', groups='run' = leave-one-run-out inside each subject). Runs through the _ItemTask/_apply machinery: subject-level parallelism, progress, and path-backed caching. cache=True writes one predict bundle (.h5) per subject — the result's ingredients (arrays, brain maps, model spec for refit), never a pickled estimator; the in-memory results mirror the bundle (estimator=None) so resumed sessions see identical fields. write_predict_bundle/read_predict_bundle join the fit-bundle IO in execution.py under the shared schema version. BREAKING: predict(y=...) previously raised (and before that, aggregated across subjects — that operation is predict_group since the phase-1 carve-out). predict() with no arguments now decodes stored .Y instead of raising for a missing X_new. Closes #478 phases 2-3.
…tion model, vocabulary Migration guide's #478 section now documents the finished state: predict(y=) per-subject decoding -> PredictCollection, predict_group as the aggregate, the 'loso'/'loro' -> 'logo'/'loo' rename, and the shared resolve_cv groups fix at both levels. execution-model.md gains the predict-bundle section (ingredients-only, no pickled estimators) and folds predict(y=) into the path-backed/cache story. api-vocabulary.yml gains the `cv` row; tables and API sources regenerated. Also removes the stale collection pipeline/pipesteps API pages and their build_api_docs.py + myst.yml entries — those modules died with the legacy cv() pipeline but the generator config kept resurrecting the pages. Part of #478 phases 2-3.
…iring fixed in batched phase randomization poe test-all (not run since the GPU-layer refactor) surfaced two real bugs in the timeseries GPU path plus stale slow tests: - circle_shift GPU derived shift amounts via RandomState(seed).choice( arange(n)) — a different draw than circle_shift()'s randint(1, n), and one that could produce an identity shift. Same seed gave different permutations on GPU vs CPU, breaking the deterministic cross-backend RNG contract. New _circle_shift_amounts() makes exactly the CPU draw. - _phase_randomize_gpu_batched applied an extra .flip() to the negative- frequency phases. pos_freq/neg_freq are built already in conjugate order, so flipping mispairs conjugates: the spectrum goes non-Hermitian and .real of the ifft silently distorts the surrogate — statistically wrong, not just nondeterministic. (The non-batched GPU version was correct; only the batched path the permutation test uses had the flip.) Both pinned by new TestGpuDrawIdentity unit tests (per-seed CPU/GPU draw identity); the two slow GPU-vs-CPU tests now pass at their original tight tolerance. inference-internals.md documents the pairing invariant. BREAKING: GPU null distributions for timeseries_correlation_permutation_ test change (they now equal the CPU nulls for a given seed). Stale slow tests corrected alongside: test_threshold now asserts the documented v0.6.0 zero-aware "98%" percentile semantics, and the two test_matches_stats_py_* cross-checks are deleted — their comparison target (correlation_permutation_test(method=...)) was deliberately split into the timeseries function and no longer exists. Full suite (poe test-all): 2395 passed, 0 failed, 5 skipped (CUDA-gated).
…categories preserved
Warnings raised inside _apply workers (RankDeficientDesignWarning above
all) died on the loky worker's stderr: invisible to catch_warnings,
pytest.warns, and filterwarnings("error") in the parent, usually invisible
entirely in notebooks, and suppressed on repeat runs by the reused
workers' once-per-location filter. A rank-deficient design could fit
silently at n_jobs>1.
_wrap_worker now captures warnings as pickle-safe _WorkerWarning records
(category as import-path strings, never a class object) and _apply relays
them via warnings.warn_explicit: deduplicated by (category, message)
across subjects with a who-raised-it annotation, real categories preserved
so parent-side filters and pytest.warns work, UserWarning fallback (class
name kept in text) for unimportable categories. The serial n_jobs=1 path
uses the same capture/relay, so behavior is identical at any n_jobs.
Documented caveats (execution-model.md): filterwarnings("error") promotes
at relay time after the parallel step, and warnings preceding a worker
exception are dropped in favor of the error.
…owser support to 0.6.1 Tutorials are now plain marimo notebooks (PEP 723 header = marimo + nltools) for local editing and molab; every IN_WASM / micropip / seed_resources / browser_* cell is gone. `docs-preview` runs `myst start --execute` so pages show baked outputs from the execute cache instead of silently re-rendering without them. Fixed two tutorials that raised mid-run (halting every later cell): 04_isc passed metric="median" (now summary="median"); 02_design_matrix appended exact duplicate columns, which append() now refuses — it appends jittered copies. BREAKING: removed nltools.templates.seed_resources and the Pyodide/IDBFS fetch path, nltools.datasets.PAIN_RESOURCES / EMOTION_METADATA / emotion_resources, scripts/build_marimo_wasm.py, the docs-wasm and test-pyodide poe tasks, the pyodide CI job, and nltools/tests/pyodide. All of it is preserved on the 0.6.1-browser branch.
Plot cells ended with plt.gcf() / _fig as their last expression, so the figure was emitted by the rich display and again by the pyplot end-of-cell flush (in both marimo and the docs' ipykernel). End the cells on a statement instead.
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.
Rolling branch for the remaining 0.6.0 fixes. Rather than juggling the scattered open PRs, work lands here incrementally and this PR supersedes them one at a time as their content is absorbed and finalized.
Landed so far
Nilearn skill drift (supersedes #477)
find_spikes / DesignMatrix regressor hygiene (supersedes #472)
Carries both original fixes from #472 (duplicate spike regressors from the two detectors; the column-less design matrix losing its row count), plus the review-driven rework:
clean=is gone — dedup is unconditional. The colliding detections are bitwise-identical one-hot columns, so dedup only decides which name survives (global_spike, deterministic tie-break). No information is at stake, so there is no opt-out: an escape hatch would only manufacture straight duplicate columns, which the design layer now refuses outright.append(axis=1)raises on bitwise-duplicate columns (identical values under any names, compared on Float64 bytes so an int one-hot and its float twin match), just as duplicate names already did. A design with straight duplicate columns is rank deficient by construction — the model over it is not computable — and silently keeping one copy would be a modeling decision made on the user's behalf. Only duplication introduced by the append is checked; a base matrix that already contains duplicates is left alone. BREAKING relative to master.n_rowsis now a real contract: it survivescopy()/copy_with()/ the copy-constructor (with a fallback to the source's height when a transform empties the frame), a conflicting or negative value raises instead of being silently ignored, andto_numpy()/np.asarray()return(n, 0)instead of(0, 0)for a column-less matrix.Decisions resolved
global_spikewins by default; the column values are identical either way.Docs: migration guide is updated; API reference regeneration (
poe docs-generate) is deferred to a batched pass before merge.Also landed (detail in the comments below)
RankDeficientDesignWarning.progress_barand keyword-only enforcement, standardized library-wide.nltools.statsremoved and consolidated intonltools.algorithms; ISC vocabulary canonicalized;cluster_summary/extract_roirenames; manifest-driven vocabulary enforcement (api-vocabulary.yml+check_api_vocabulary.pyinlint-api); one canonicaltail=vocabulary across every p-value..nl_namespace (.nl_poly_0,.nl_cosine_1,.nl_global_spike1,.nl_r0_poly_0), so detection keys on a prefix nltools controls instead of pattern-matching user-controlled names. Fixes theadd_poly()false positive on 24-parameter motion confounds and a second bug of the same shape invif()..write()to.csvemitted tab-separated data its own reader couldn't parse, and.h5had no reader at all. Both fixed; neither path had test coverage before.Remaining before merge
Batched docs regeneration (
poe docs-generate+poe changelog). Everything else — #479 (iplotautoscaling), #478 (BrainCollection.predictmap-reduce), #484 (GPU survey) — is scoped in the current status comment; #478 phase 2 and #484 are expected to be fast-follow PRs rather than blockers.