[FIX] ASR: asr_calibrate must pass step_sizes to fit_eeg_distribution#109
Merged
Merged
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 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. 🚀 New features to boost your workflow:
|
Collaborator
Author
|
Note: includes the |
Closed
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
force-pushed
the
fix/asr-calibrate-step-sizes
branch
from
July 19, 2026 09:17
2551a60 to
d24300b
Compare
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
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.
fit_eeg_distribution'sstep_sizesdefault was[0.0220, 0.6000], an accidental copy of thefit_quantilesdefault. That value collapses the scale grid search to essentially a single point, soasr_calibrate(which calledfit_eeg_distributionwithoutstep_sizes) inherited a degenerate grid, biasing the fittedmu/sigand hence the threshold matrixT. The function's own docstring already documentsdefault=[0.01, 0.01], matching the valueclean_windowspasses explicitly.Correct the signature default to match the docstring, and have
asr_calibratepassstep_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.npyfixture 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. Fulltests/test_asr.pypasses;ruffclean.