FIX: Add on_few_samples parameter to core rank estimation #13350
Conversation
Signed-off-by: Emmanuel Ferdman <[email protected]>
|
Hello! 👋 Thanks for opening your first pull request here! ❤️ We will try to get back to you soon. 🚴 |
|
Wow, this causes more than 30 tests to fail. Will need some investigation as to which ones are legitimate failures and how to correct them |
|
We should probably turn it into a warning instead |
emmanuel-ferdman
changed the title
Signed-off-by: Emmanuel Ferdman <[email protected]>
for more information, see https://pre-commit.ci
mne/rank.py
Outdated
| @@ -184,7 +184,7 @@ def _estimate_rank_meeg_signals( | |||
| """ | |||
| picks_list = _picks_by_type(info) | |||
| if data.shape[1] < data.shape[0]: | |||
| ValueError( | |||
| warn( | |||
There was a problem hiding this comment.
From talking to @drammock today, it might actually be safest to have a on_few_samples="raise" option in public-facing functions/APIs that eventually gets passed to this function. Then you can use _on_missing(...) helper function to throw an error or emit a warning. Would you be up for trying this?
Then in the tests below you should be able to pass on_few_samples="ignore" rather than adding a pytest warning ignore decorator
There was a problem hiding this comment.
@larsoner sounds good, I think I got it and implemented it. Please let me know if I missed anything.
Signed-off-by: Emmanuel Ferdman <[email protected]>
emmanuel-ferdman
changed the title
for more information, see https://pre-commit.ci
| @@ -582,6 +583,7 @@ def compute_raw_covariance( | |||
| return_estimators=False, | |||
| reject_by_annotation=True, | |||
| rank=None, | |||
| on_few_samples="warn", | |||
There was a problem hiding this comment.
Nowadays we try to add * to functions (and we've been adding them to old functions as we come across them), like
*,
on_few_samples="warn",
But really we could probably go with something much farther up... maybe after picks and before method?
| on_few_samples : str | ||
| Can be 'warn' (default), 'ignore', or 'raise' to control behavior when | ||
| there are fewer samples than channels, which can lead to inaccurate | ||
| covariance or rank estimates. |
There was a problem hiding this comment.
| covariance or rank estimates. | |
| covariance or rank estimates. | |
| .. versionadded:: 1.11 |
| @@ -872,6 +878,7 @@ def compute_covariance( | |||
| return_estimators=False, | |||
| on_mismatch="raise", | |||
| rank=None, | |||
| on_few_samples="warn", | |||
There was a problem hiding this comment.
*,
on_few_samples="warn",
Here maybe after projs would make sense?
| on_few_samples : str | ||
| Can be 'warn' (default), 'ignore', or 'raise' to control behavior when | ||
| there are fewer samples than channels, which can lead to inaccurate | ||
| covariance or rank estimates. |
There was a problem hiding this comment.
| covariance or rank estimates. | |
| covariance or rank estimates. | |
| .. versionadded:: 1.11 |
| @@ -1751,6 +1768,10 @@ def prepare_noise_cov( | |||
|
|
|||
| dict(mag=1e12, grad=1e11, eeg=1e5) | |||
| %(on_rank_mismatch)s | |||
| on_few_samples : str | |||
There was a problem hiding this comment.
Only functions that compute a covariance should end up warning about the number of samples. Things like prepare_noise_cov and regularize shouldn't have any new public parameter. Under the hood they should pass on_few_samples="ignore" (or maybe nothing should need to be passed for it to work?)
| with pytest.warns(RuntimeWarning, match="Too few samples"): | ||
| cov = compute_raw_covariance(raw) | ||
| cov = compute_raw_covariance(raw, on_few_samples="ignore") |
There was a problem hiding this comment.
Maybe leave this one as it is on main / revert this change to test that a warning is emitted properly?
|
in addition to @larsoner's comments above, this PR also still needs a changelog entry in |
PR Summary
This small PR warns on rank estimate with too few samples.