-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Avoid lowpass=0 in brainvision data #10517
Conversation
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.
no opinion here. @sappelhoff thoughts?
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 have never used a V-Amp or analyzed data produced by it, but the proposed changes look reasonable to me. If @Lychfindel can vouch for the V-Amp data being organized this way, I think it's fine to go ahead.
@Lychfindel did you record the data yourself and made sure the 0
gets entered when you
have a BrainVision Recorder setting to disable software filters?
Note that hardware filters are surely still in place, even if neither the data nor the recording interface (I assume BrainVision Recorder) tells you about that. That's just a necessity to prevent aliasing of the signal when digitizing to some sampling frequency.
I guess we need to distinguish 3 types of filters:
- hardware --> this filters the data during / prior to digitization of the analogue signal
- software 1 --> this is applied in the recording interface (e.g. BrainVision recorder) and directly applied to the recorded data, cannot be reversed
- software 2 --> this is applied e.g., in MNE-Python to already recorded and saved data ... can be changed and reversed at will
Example for 2. --> the Brain Products "Brain AMP DC" always samples at 5kHz -- even when you set the sampling frequency to 1000Hz in the recording interface. "Under the hood", the recording interface just applies an additional software filter and downsampling process prior to saving the data. This has been confirmed to me by Brain Products in a personal Email. :-)
Yes, I recorded the data myself even last week, and I always disable software filters. The schema in the hdr file is always the same:
|
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.
cool, then I'd say: please add a detailed changelog entry about this bugfix and we can merge. CI failures seem unrelated
If you wanna go the extra mile, you'd record a new datafile with software filters ON, and check how that looks in the data and whether we can read it in mne.
In the V-Amp is not possible to set hardware filters, but I did three more test recordings: without software filters, with software filters, with software filters except in one channel. All of them are correctly read by mne and in the set with one channel not filtered I receive the expected warnings
|
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.
one nitpick, other than that, +1 to merge
Co-authored-by: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
@@ -426,6 +426,8 @@ | |||
|
|||
.. _Senwen Deng: https://snwn.de | |||
|
|||
.. _Alessandro Tonin: https://wysscenter.ch/team/alessandro-tonin |
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.
@Lychfindel I used the :newcontrib:
above and realized we needed an entry in names.inc
, feel free to push to change the URL if this is not the one you want!
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.
Sure thanks a lot, then I will remove it from my other PR!
🎉 Congrats on merging your first pull request! 🥳 Looking forward to seeing more from you in the future! 💪 |
Thanks @Lychfindel ! |
* upstream/main: (40 commits) FIX: Flake (mne-tools#10540) FIX: Correct link (mne-tools#10536) DOC: Update installers (mne-tools#10535) ENH: Add dark mode to website (mne-tools#10523) WIP: Copy BEM surfaces by default (don't symlink) (mne-tools#10531) Avoid lowpass=0 in brainvision data (mne-tools#10517) DOC: Update installers [skip azp] [skip actions] (mne-tools#10528) FIX: Fix for old build (mne-tools#10527) Fix line noise at wrong frequencies (mne-tools#10525) FIX : read fids in eeglab (mne-tools#10521) MAINT: Prefer PySide6 in testing (mne-tools#10513) ENH: Add overview_mode support (mne-tools#10501) MRG: Updates for qtpy in mne-qt-browser (mne-tools#10509) BUG: Fix bug with themes on macOS (mne-tools#10500) MAINT: Bump installer links (mne-tools#10511) Add metadata to combine_channels (mne-tools#10504) MAINT: Standardize tests (mne-tools#10502) CI: Test circle (mne-tools#10506) ENH: Use HiDPI splash screen on HiDPI screens (mne-tools#10503) WIP,MNT: Add support for QtPy (mne-tools#10430) ...
* upstream/main: (52 commits) MAINT: Extra test for coreg (mne-tools#10549) BUG: Fix annot meas_date / crop (mne-tools#10491) Update latest.inc Use liblinear solver instead of lbgfs in all decodung examples [skip azp][skip actions] (mne-tools#10552) STY: Hotfix for HTML [ci skip] Allow making inverse solutions from fixed-orientation discrete forward models, enabling multi-dipole modeling (mne-tools#10464) Correct documented default number of CV splits [skip azp][skip actions] (mne-tools#10548) MRG: Add error checking to prevent the creation of montage with invalid [x, y, z] (mne-tools#10547) MRG: Add "verbose" parameter to pick_channels() method (mne-tools#10544) CI: Avoid bad PySide6 (mne-tools#10545) FIX: Flake (mne-tools#10540) FIX: Correct link (mne-tools#10536) DOC: Update installers (mne-tools#10535) ENH: Add dark mode to website (mne-tools#10523) WIP: Copy BEM surfaces by default (don't symlink) (mne-tools#10531) Avoid lowpass=0 in brainvision data (mne-tools#10517) DOC: Update installers [skip azp] [skip actions] (mne-tools#10528) FIX: Fix for old build (mne-tools#10527) Fix line noise at wrong frequencies (mne-tools#10525) FIX : read fids in eeglab (mne-tools#10521) ...
Reference issue
Example: Fixes #10516 .
What does this implement/fix?
If in the header high cutoff is 0, then the lowpass filter is considered as disabled.