-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
BUG: Raise early on non-finite values in PSD (Welch) and ICA.fit (Fix… #13486
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Hello! 👋 Thanks for opening your first pull request here! ❤️ We will try to get back to you soon. 🚴 |
|
Hi! First-time contributor here. I’ve rebased on current main, pre-commit passes locally, and targeted tests are green: pytest -q mne/time_frequency/tests -k psd → 21 passed, 6 skipped Could someone please approve CI to run on this PR? Thanks! |
doc/changes/devel/13364.bugfix.rst
Outdated
| @@ -0,0 +1,2 @@ | |||
| Fail early with a clear error when non-finite values (NaN/Inf) are present | |||
| in PSD (Welch) and in ICA.fit, avoiding deep assertion failures (GH-13364). | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| in PSD (Welch) and in ICA.fit, avoiding deep assertion failures (GH-13364). | |
| in PSD (Welch) and in ICA.fit, avoiding deep assertion failures, by :newcontrib:`First Last` |
then also add to doc/changes/names.inc
|
Hi, I revised the contritbution doc - anything else I need to do for the workflow to be approved? |
drammock
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made a few comments/suggestions about specific bits of the code/tests. Additional high-level comments:
#13364 (comment) suggested that we error for ICA.fit when there are NaNs present, but for PSD we should warn if NaNs are on specific channels and compute the PSD for the other channels that have good data. I see two new ValueErrors and no new warnings in the time_frequency code, so it seems like you're doing something different from what @larsoner recommended.
| Fail early with a clear error when non-finite values (NaN/Inf) are present | ||
| in PSD (Welch) and in ICA.fit, avoiding deep assertion failures by :newcontrib: `Emma Zhang` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Fail early with a clear error when non-finite values (NaN/Inf) are present | |
| in PSD (Welch) and in ICA.fit, avoiding deep assertion failures by :newcontrib: `Emma Zhang` | |
| Improved error message when non-finite values (NaN/Inf) are detected in calls to | |
| :meth:`inst.compute_psd(method="welch") <mne.io.Raw.compute_psd>` or | |
| :meth:`ICA.fit() <mne.preprocessing.ICA.fit>`, by :newcontrib:`Emma Zhang`. |
| rng = np.random.RandomState(1) | ||
| data = rng.randn(4, 1000) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| rng = np.random.RandomState(1) | |
| data = rng.randn(4, 1000) | |
| rng = np.random.default_rng(1) | |
| data = rng.standard_normal(size=(4, 1000)) |
this is the pattern recommended for new code; see https://numpy.org/doc/stable/reference/random/index.html#random-quick-start
| raw = RawArray(data.copy(), info) | ||
| raw._data[1, 50] = np.inf | ||
| ica = ICA(n_components=2, random_state=0, method="fastica", max_iter="auto") | ||
| with pytest.raises(ValueError, match=r"Input data contains non[- ]?finite values"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| with pytest.raises(ValueError, match=r"Input data contains non[- ]?finite values"): | |
| with pytest.raises(ValueError, match=r"Input data contains non-finite values"): |
since you wrote the error message, we know exactly what it says; no need for the [- ]?
| rng = np.random.RandomState(0) | ||
| x = rng.randn(1, n_samples) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see prior comment re: default_rng and standard_normal
| x = rng.randn(2, n_samples) | ||
| # NaN only in ch0; ch1 has no NaN => masks not aligned -> should raise | ||
| x[0, 500] = np.nan | ||
| with pytest.raises(ValueError, match="aligned|not aligned|non[- ]?finite|NaN|Inf"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
similar comment as above: avoid complicated regex in the match parameter; we wrote the error message so we can pull out a distinctive string of words from it and match on that. That approach makes maintenance easier (you can copy/paste/search on the match string to find the place in the codebase where the error is raised)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the filename needs to match the PR, not the issue. so move this file to 13486.bugfix.rst
…es #13364)
Reference issue (if any)
Fixes #13364
What does this implement/fix?
psd_array_welch
ICA.fit
Additional information