Skip to content

Conversation

@hoechenberger
Copy link
Member

@hoechenberger hoechenberger commented May 10, 2022

This is an attempt to fix the issue reported at https://mne.discourse.group/t/saving-epoch-error-no-eeg-channel-found

WIP:

  • tests
  • changelog
  • update docstring to explain we're removing rejection thresholds for EEG

@hoechenberger hoechenberger added the backport-candidate on-merge: backport to maint/1.11 label May 10, 2022
@hoechenberger
Copy link
Member Author

If anyone's willing to take this one over, please feel free to do so as I'm very much busy with other stuff

cc @larsoner @adam2392 @sappelhoff @alexrockhill

@hoechenberger hoechenberger added this to the 1.1 milestone May 10, 2022
@alexrockhill alexrockhill marked this pull request as ready for review May 10, 2022 21:28
@alexrockhill
Copy link
Contributor

@hoechenberger, I didn't update the docstring, I'm really not sure what it would say, I don't think this is a user-facing change. I think the behavior is now what's expected as in the epochs can be saved and loaded from a disk with a rejection threshold applied. You could add something to explain about that your rejection threshold for eeg would be lost, but I don't think a user would expect that data to still be there since the data is no longer in that form anyway and it's not really of any use since the units changed...

@hoechenberger
Copy link
Member Author

Great work, @alexrockhill! And I agree, I think we don't need a docstring update after all. Feel free to merge once CI goes green!

Co-authored-by: Richard Höchenberger <richard.hoechenberger@gmail.com>
@alexrockhill
Copy link
Contributor

alexrockhill commented May 10, 2022

Is there a fix to the pandas install in the works or are we merging without those tests?

@hoechenberger
Copy link
Member Author

I'd leave this decision to @larsoner
As I'm going to bed now anyway 😁😴

@alexrockhill alexrockhill merged commit 820bd7a into mne-tools:main May 11, 2022
@alexrockhill
Copy link
Contributor

Thanks @hoechenberger!

@hoechenberger hoechenberger deleted the csd-reject branch May 11, 2022 04:16
@hoechenberger
Copy link
Member Author

Thanks for bringing this home!

Comment on lines +182 to +183
inst.reject = None if inst.reject is None else \
{k: v for k, v in inst.reject.items() if k != 'eeg'}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if inst.reject and "eeg" in inst.reject:
   del inst.reject["eeg"]

would be more readable.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea!
I'll push a "fix"

hoechenberger added a commit to hoechenberger/mne-python that referenced this pull request May 11, 2022
@hoechenberger hoechenberger mentioned this pull request May 11, 2022
hoechenberger added a commit that referenced this pull request May 11, 2022
hoechenberger added a commit to hoechenberger/mne-python that referenced this pull request May 11, 2022
…ne-tools#10619)

* Drop EEG rejection thresholds when replacing EEG with CSD channels

This is an attempt to fix the issue reported at
https://mne.discourse.group/t/saving-epoch-error-no-eeg-channel-found

* fix, working version

* Update doc/changes/latest.inc

Co-authored-by: Richard Höchenberger <richard.hoechenberger@gmail.com>

Co-authored-by: Alex <aprockhill@mailbox.org>
hoechenberger added a commit to hoechenberger/mne-python that referenced this pull request May 11, 2022
larsoner added a commit that referenced this pull request May 11, 2022
* Drop EEG rejection thresholds when replacing EEG with CSD channels (#10619)

* Drop EEG rejection thresholds when replacing EEG with CSD channels

This is an attempt to fix the issue reported at
https://mne.discourse.group/t/saving-epoch-error-no-eeg-channel-found

* fix, working version

* Update doc/changes/latest.inc

Co-authored-by: Richard Höchenberger <richard.hoechenberger@gmail.com>

Co-authored-by: Alex <aprockhill@mailbox.org>

* Simplify #10619 (#10620)

* MAINT: Fix

* FIX: Fix for regression

* FIX: Fix for pandas

* FIX: Better fix

* FIX: Adjust

Co-authored-by: Alex <aprockhill@mailbox.org>
Co-authored-by: Eric Larson <larson.eric.d@gmail.com>
larsoner added a commit to HanBnrd/mne-python that referenced this pull request May 17, 2022
* upstream/main: (24 commits)
  Use less memory when loading EDF file (mne-tools#10638)
  BUG: fix ipython console accessibility after MNEQtBrowser in Spyder (mne-tools#10637)
  WIP: Fix mne.time_frequency.multitaper Nyquist adjustment slightly incorrect (mne-tools#10541)
  BUG: Fix bug with fNIRS reordering (mne-tools#10630)
  MNT: PyQt6 for pip pre 3.10 (mne-tools#10636)
  CI: Fix conda (mne-tools#10628)
  MRG: Update installer links to point to 1.0.3_0 (mne-tools#10622)
  [BUG, MRG] fix info write access (mne-tools#10626)
  [MRG] Fixed group_by titles in evoked_plot. (mne-tools#10618)
  [BUG, MRG] Replace image_interp with interpolation in topomap plot arguments (mne-tools#10617)
  MAINT: Fix timeout (mne-tools#10624)
  Simplify mne-tools#10619 (mne-tools#10620)
  Drop EEG rejection thresholds when replacing EEG with CSD channels (mne-tools#10619)
  Add get_montage support for fNIRS (mne-tools#10611)
  ENH: Improve make_head_surface options (mne-tools#10592)
  [ENH, MRG] Add citation for intracranial JOSS paper (mne-tools#10616)
  [ENH] Add sys info for mne-icalabel (mne-tools#10615)
  MRG: Add "highlight" parameter to Evoked.plot() to conveniently highlight time periods (mne-tools#10614)
  MRG: Allow to pass array of "average" values to plot_evoked_topomap() (mne-tools#10610)
  fix: snirf length units (mne-tools#10613)
  ...
@hoechenberger hoechenberger added backported and removed backport-candidate on-merge: backport to maint/1.11 labels Jun 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants