EGI/MFF events outside EEG recording should not break the code#11505
Merged
agramfort merged 3 commits intomne-tools:mainfrom Feb 25, 2023
Merged
EGI/MFF events outside EEG recording should not break the code#11505agramfort merged 3 commits intomne-tools:mainfrom
agramfort merged 3 commits intomne-tools:mainfrom
Conversation
Some EGI events can be outside (before or after) the EEG recording. E.g. for video-synchronization markers or "IEND" codes. In this case, the code breaks with an "index out of bounds" error. Examples are shown in the forum: https://mne.discourse.group/t/index-error-when-trying-to-read-data/2823 https://mne.discourse.group/t/index-error-while-reading-data/5535 I had a similar issue with an EEG with video segments terminating later then the EEG recording. The edits here should make sure to skip events that are placed illegally, i.e. below 0 (see forum example) or after the EEG end (other forum example+my case). This edit fixed the problem for me and should not harm the usual function. However, out-of-bounds events will be skipped/lost. As alternative, the length of the events array could be extended for out of bounds events, but this may cause other issues with synchronizing to the EEG data.. Hence this option seems preferable. A warning may be added to notify the user of why an event was skipped.
|
Hello! 👋 Thanks for opening your first pull request here! ❤️ We will try to get back to you soon. 🚴🏽♂️ |
agramfort
reviewed
Feb 24, 2023
makes sense ;) Co-authored-by: Alexandre Gramfort <alexandre.gramfort@m4x.org>
|
🎉 Congrats on merging your first pull request! 🥳 Looking forward to seeing more from you in the future! 💪 |
larsoner
added a commit
to larsoner/mne-python
that referenced
this pull request
Mar 1, 2023
* upstream/main: (264 commits) BUG: Fix deprecated API usage in example (mne-tools#11512) Deprecate 'kind' and 'path' in favor of 'fname' in the layout reader (mne-tools#11500) EGI/MFF events outside EEG recording should not break the code (mne-tools#11505) fixed annotations error on export (mne-tools#11435) DOC: Update installer links [skip azp] [skip actions] [skip cirrus] (mne-tools#11506) BUG: updates for MPL 3.7 compatibility (mne-tools#11409) Fix docstrings by replacing str with path-like and fix double backticks for formatting (mne-tools#11499) Use pathlib.Path instead of os.path to handle files and folders [circle deploy] (mne-tools#11473) MAINT: Fix Circle [circle deploy] (mne-tools#11497) MAINT: Use mamba in CIs (mne-tools#11471) Updating documentation to clarify full vs half-bandwidth and defaults in time_frequency.multitaper.py (mne-tools#11479) Fix typo in tutorial (mne-tools#11498) Typo fix and added colons before code (mne-tools#11496) [MRG] ENH/DOC: demo custom spectrum creation (mne-tools#11493) Accept only left-clicks for adding annotations (mne-tools#11491) [BUG, MRG] Fix pial surface loading, logging in locate_ieeg (mne-tools#11489) [ENH] Added unit_role to add non-breaking space between magnitude and units (mne-tools#11469) MAINT: Fix CircleCI build (mne-tools#11488) [DOC] Updated decoding.SSD documentation and internal variable naming (mne-tools#11475) Typo fix (mne-tools#11485) ...
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
2 participants
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.
Some EGI events can be outside (before or after) the EEG recording. E.g. for video-synchronization markers or "IEND" codes. In this case, the code breaks with an "index out of bounds" error. Examples are shown in the forum: https://mne.discourse.group/t/index-error-when-trying-to-read-data/2823 https://mne.discourse.group/t/index-error-while-reading-data/5535 I had a similar issue with an EEG with video segments terminating later then the EEG recording.
The edits here should make sure to skip events that are placed illegally, i.e. below 0 (see forum example) or after the EEG end (other forum example+my case). This edit fixed the problem for me and should not harm the usual function. However, out-of-bounds events will be skipped/lost. As alternative, the length of the events array could be extended for out of bounds events, but this may cause other issues with synchronizing to the EEG data.. Hence this option seems preferable. A warning may be added to notify the user of why an event was skipped.
Thanks for contributing a pull request! Please make sure you have read the
contribution guidelines
before submitting.
Please be aware that we are a loose team of volunteers so patience is
necessary. Assistance handling other issues is very welcome. We value
all user contributions, no matter how minor they are. If we are slow to
review, either the pull request needs some benchmarking, tinkering,
convincing, etc. or more likely the reviewers are simply busy. In either
case, we ask for your understanding during the review process.
Again, thanks for contributing!
Reference issue
Example: Fixes #1234.
What does this implement/fix?
Explain your changes.
Additional information
Any additional information you think is important.