Skip to content

fix(inference)!: add progress_bar to the permutation and bootstrap family - #473

Closed
ljchang wants to merge 1 commit into
masterfrom
fix/inference-progress-bar
Closed

fix(inference)!: add progress_bar to the permutation and bootstrap family#473
ljchang wants to merge 1 commit into
masterfrom
fix/inference-progress-bar

Conversation

@ljchang

@ljchang ljchang commented Jul 29, 2026

Copy link
Copy Markdown
Member

Problem

The inference functions print 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 — emits one progress bar per call.

This came up while writing the dartbrains thresholding chapter, where a 100-iteration max-statistic calibration loop produced 100 progress bars. The only workaround was wrapping every call in contextlib.redirect_stderr.

It also contradicts our own canonical kwarg table in CLAUDE.md:

Concept Canonical kwarg Notes
Progress indicator progress_bar: bool = False not show_progress, verbose

BrainCollection, LocalAlignment, and the ridge solvers already follow it. The inference family was the gap.

Change

Adds progress_bar: bool = False across the family, and replaces fourteen hand-rolled tqdm sites with two shared helpers in algorithms/inference/utils.py:

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, keeping it off the import path when unused.

Threaded through both layers, since nltools.stats re-exports thin wrappers (they translate parallel=device=) that are distinct function objects from the algorithm-layer functions — a wrapper accepting the kwarg is not the same as forwarding it:

  • 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.

The alternative — defaulting to True to preserve behavior — was considered and rejected: it would contradict the convention table and leave the family internally inconsistent. A library shouldn't write to stderr unasked.

Tests

New nltools/tests/core/test_inference/test_progress_bar.py, parametrized over both layers, asserting that:

  1. the kwarg exists,
  2. it defaults to False,
  3. nothing reaches stderr by default, and
  4. progress_bar=True still produces a bar — so the knob is a real toggle, not a silent no-op.

Point 3 caught a real bug during development: in correlation.py I initially inserted progress_bar before single_feature in a helper signature, and the positional dispatch call silently shifted single_feature's value into it. ty caught a second one — an un-threaded progress_bar in the correlation GPU batching path, which no test could reach without CUDA.

  • uv run poe lint — clean (ruff + ty)
  • uv run poe test — 1702 passed

🤖 Generated with Claude Code

https://claude.ai/code/session_01SSqvVqrZ7ZPXdQHH3Hivkr

…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).
@ejolly

ejolly commented Aug 20, 2026

Copy link
Copy Markdown
Collaborator
  • First check that progress bar is not affecting implementation speed (e.g. due to updates)
  • Keep off, and standardize across library

@ejolly

ejolly commented Aug 20, 2026

Copy link
Copy Markdown
Collaborator

Superseded by #483 — the branch was merged into 0.6.0-fixes as-is and then reworked per review: bar overhead measured (+1.6–4% on real workloads, confirming off-by-default), maybe_tqdm/make_progress_bar promoted to nltools/utils.py and every hand-rolled tqdm site converted (single library-wide mechanism, tqdm.auto so notebooks get widget bars), the facades that silently lost their bars (Adjacency.similarity/.ttest/.bootstrap, BrainData.bootstrap) re-threaded, and a migration-guide entry added for the isc_* default flip. Details: #483 (comment)

@ejolly ejolly closed this Aug 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants