MRG: Return empty list of SSP projectors if no ECG, EOG events were found #9277
Conversation
|
I'm not sure if we should have a deprecation cycle though, WDYT? |
|
no deprecation is ok
… |
hoechenberger
changed the title
hoechenberger
changed the title
doc/changes/latest.inc
Outdated
|
|
||
| - Fitting `~mne.preprocessing.ICA` on baseline-corrected `~mne.Epochs`, and / or applying it on baseline-corrected `~mne.Epochs` or `~mne.Evoked` data will now display a warning. Users are advised to only baseline correct their data after cleaning is completed (:gh:`9033` by `Richard Höchenberger`_) | ||
|
|
||
| - `mne.preprocessing.compute_proj_eog` and `mne.preprocessing.compute_proj_ecg` now return empty lists if no EOG or ECG events, respectively, could be found. Previously, we'd return ``None`` in these situations, which does not match the documented behavior of returning a list of projectors (:gh:`9277` by `Richard Höchenberger`_) |
There was a problem hiding this comment.
| - `mne.preprocessing.compute_proj_eog` and `mne.preprocessing.compute_proj_ecg` now return empty lists if no EOG or ECG events, respectively, could be found. Previously, we'd return ``None`` in these situations, which does not match the documented behavior of returning a list of projectors (:gh:`9277` by `Richard Höchenberger`_) | |
| - :func:`mne.preprocessing.compute_proj_eog` and :func:`mne.preprocessing.compute_proj_ecg` now return empty lists if no EOG or ECG events, respectively, could be found. Previously, we'd return ``None`` in these situations, which does not match the documented behavior of returning a list of projectors (:gh:`9277` by `Richard Höchenberger`_) |
There was a problem hiding this comment.
That's actually not required anymore since to some changes @drammock made to our doc building process a while ago :)
There was a problem hiding this comment.
I think this should go in BUG rather than API because it's a backward-incompatible change with no deprecation cycle
There was a problem hiding this comment.
because it's a backward-incompatible change with no deprecation cycle
Yeah that's specifically why I put it into the API section …
There was a problem hiding this comment.
I think we went through this a bit already in #9027 (comment) and #8870 (comment) but just to be more specific I think our current practice is:
- "Enhancements": new, non-breaking code changes
- "Bug": bugfixes, can immediately break existing code without any warning or deprecation cycle, but for a good reason (i.e., the existing code probably did not work correctly)
- "API": changes to the API that do not break code right away but will probably do so in the next release, and almost always emit a deprecation warning to tell you
Should we add this to the developer guide so that it's explicitly written somewhere?
There was a problem hiding this comment.
Ok I will move this to "Bug", even though I believe since this behavior has been present for so long it's expected behavior now that we're changing, so as a user I would expect this in the "API" section – also considering that the bug section is so long that it's impossible to read it in its entirety …
There was a problem hiding this comment.
for so long it's expected behavior now that we're changing
The point is not about the duration the bug has existed, but rather there is a release cycle to change your code (API) or if it breaks immediately (BUG)
as a user I would expect this in the "API" section – also considering that the bug section is so long that it's impossible to read it in its entirety …
Again please open an issue about this
There was a problem hiding this comment.
The point is not about the duration the bug has existed, but rather there is a release cycle to change your code (API) or if it breaks immediately (BUG)
Ok, got it!
|
ECG tests are failing – of course they are, I only took care of EOG tests. Will fix this ASAP. |
|
@larsoner I think this should be good to merge now! |
|
Thanks @hoechenberger ! |
* upstream/main: MRG: Return empty list of SSP projectors if no ECG, EOG events were found (mne-tools#9277) Better warning message for EDF files with annotations only (mne-tools#9283) FIX: Link [ci skip] FIX: Prepare for PyVista 0.30.0 (mne-tools#9274) Use info for checking more NIRS metadata, not raw (mne-tools#9282) MRG: Use info for checking NIRS metadata, not raw (mne-tools#9280) Fix issue where set ylim parameter gets swapped across channel types in plot_evoked_topo (mne-tools#9207)
3 participants
No description provided.