Skip to content

[FIX] ASR: asr_calibrate must pass step_sizes to fit_eeg_distribution#109

Merged
nbara merged 2 commits into
nbara:masterfrom
sappelhoff:fix/asr-calibrate-step-sizes
Jul 20, 2026
Merged

[FIX] ASR: asr_calibrate must pass step_sizes to fit_eeg_distribution#109
nbara merged 2 commits into
nbara:masterfrom
sappelhoff:fix/asr-calibrate-step-sizes

Conversation

@sappelhoff

Copy link
Copy Markdown
Collaborator

fit_eeg_distribution's step_sizes default was [0.0220, 0.6000], an accidental copy of the fit_quantiles default. That value collapses the scale grid search to essentially a single point, so asr_calibrate (which called fit_eeg_distribution without step_sizes) inherited a degenerate grid, biasing the fitted mu/sig and hence the threshold matrix T. The function's own docstring already documents default=[0.01, 0.01], matching the value clean_windows passes explicitly.

Correct the signature default to match the docstring, and have asr_calibrate pass step_sizes=[0.01, 0.01] explicitly so its behavior no longer depends on the default.

Testing

Added a regression test that builds a windowed-RMS vector from the eeg_raw.npy fixture and asserts the default fit now matches an explicit [0.01, 0.01] call while diverging materially from the old [0.022, 0.6] default. Full tests/test_asr.py passes; ruff clean.

@codecov

codecov Bot commented Jul 13, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.11%. Comparing base (107cd1b) to head (d987300).

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #109   +/-   ##
=======================================
  Coverage   83.11%   83.11%           
=======================================
  Files          25       25           
  Lines        2825     2825           
=======================================
  Hits         2348     2348           
  Misses        477      477           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@sappelhoff

Copy link
Copy Markdown
Collaborator Author

Note: includes the nonlinear_eigenspace reproducibility fix from #118. test_asr_class[*-riemann] compares incremental-vs-bulk output with a loose tolerance and is flaky on CI because that path used an unseeded RNG; this can trip on any PR regardless of what it changes. #118 is the root fix — once it merges first, this commit drops out on rebase.

@sappelhoff sappelhoff mentioned this pull request Jul 14, 2026
fit_eeg_distribution's step_sizes default was [0.0220, 0.6000], an
accidental copy of the fit_quantiles default. That value collapses the
grid search over dropout/scale to essentially a single point, so
asr_calibrate (which called fit_eeg_distribution without step_sizes)
inherited a degenerate grid, biasing the fitted mu/sig and hence the
threshold matrix T.

fit_eeg_distribution's own docstring already documents
default=[0.01, 0.01] for step_sizes, matching the value clean_windows
already passes explicitly. Correct the signature default to match the
docstring, and have asr_calibrate pass step_sizes=[0.01, 0.01]
explicitly (mirroring clean_windows) so its behavior no longer depends
on the buggy default.

Added a regression test that builds a windowed-RMS vector from the
eeg_raw.npy fixture (mirroring asr_calibrate's own windowing) and
asserts that fit_eeg_distribution's default step_sizes now matches an
explicit [0.01, 0.01] call, while the old [0.022, 0.6] default
diverges materially.
@sappelhoff
sappelhoff force-pushed the fix/asr-calibrate-step-sizes branch from 2551a60 to d24300b Compare July 19, 2026 09:17
@nbara nbara closed this in 1c38cd5 Jul 20, 2026
@nbara nbara reopened this Jul 20, 2026
@nbara
nbara merged commit ba7aa78 into nbara:master Jul 20, 2026
7 checks passed
@sappelhoff
sappelhoff deleted the fix/asr-calibrate-step-sizes branch July 20, 2026 09:02
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