Skip to content

Conversation

@christianbrodbeck
Copy link
Member

What does this implement/fix?

I've been given a bunch of brainvision files with software filter settings in the VHDR file. Loading failed because the reader seems to assume that the first column contains channel names, when in fact it looks like it contains indices:

S o f t w a r e  F i l t e r s
==============================
#     Low Cutoff [s]   High Cutoff [Hz]   Notch [Hz]
1      10               125                Off
2      10               125                Off
3      10               125                Off
...

This PR fixes that for my files. However, I noticed that none of the testing *.vhdr files have a S o f t w a r e F i l t e r s list, they all say

S o f t w a r e  F i l t e r s
==============================
Disabled

So I don't know whether S o f t w a r e F i l t e r s always have channel indices or whether they might sometimes have names. Given that the column header is # (hard-coded on line 601) I figure always? Maybe there is someone who knows more about the format and/or has access to brainvision software could chime and generate a new testing file with some software filters?

@cbrnr cbrnr requested a review from sappelhoff April 15, 2021 05:09
@agramfort
Copy link
Member

maybe for @sappelhoff

@sappelhoff
Copy link
Member

For my own recordings I don't use the inbuilt software filters from the "BrainVision Recorder" (The Brain Products software for data recording). So all of my files and those of my colleagues show Disabled.

However I did a quick search on figshare and Gin, and found at least one file which looks like @christianbrodbeck says:

https://figshare.com/articles/dataset/EEG_and_behavioural_data_for_The_neurophysiology_of_language_processing_shapes_the_evolution_of_grammar_evidence_from_case_marking_/1394600?file=3325199

S o f t w a r e  F i l t e r s
==============================
#     Low Cutoff [s]   High Cutoff [Hz]   Notch [Hz]
1      0.3              70                 50
2      0.3              70                 50
3      0.3              70                 50
4      0.3              70                 50

@larsoner wanted to record a short XDF file using Brain Products equipment and LSL soon - Eric perhaps you could take 10 minutes more and try to save another brief recording from your amp using BrainVision Recorder, with several software filter settings switched on?

I am relatively confident though that this PR can be merged.

@hoechenberger
Copy link
Member

However, I noticed that none of the testing *.vhdr files have a S o f t w a r e F i l t e r s list, they all say

I'd like to suggest to modify one of our testing VHDR files to reflect the format you encountered, so at least we have test coverage for this case :)

@larsoner
Copy link
Member

perhaps you could take 10 minutes more and try to save another brief recording from your amp using BrainVision Recorder, with several software filter settings switched on?

I'm not sure if that machine has Recorder or if it just uses PyCorder, I'll check

@hoechenberger
Copy link
Member

I believe I also have some PyCorder data with notch filtering activated, I can have a look

@christianbrodbeck
Copy link
Member Author

I'd like to suggest to modify one of our testing VHDR files to reflect the format you encountered, so at least we have test coverage for this case :)

Do you want me to copy-paste a filter list into one of the testing files? I figured it might be safer to get an original VHDR file, because I don't know whether having software filters applied would also modify the file in other places

@cbrnr
Copy link
Contributor

cbrnr commented Apr 15, 2021

I'd try to get an original file triplet containing filter settings, otherwise we cannot be sure if we are really covering the real thing.

@cbrnr cbrnr added this to the 0.23 milestone Apr 19, 2021
@larsoner
Copy link
Member

@christianbrodbeck would it be easy for you to adapt the filters from one of your actual files and add them to one of the test files? I think this should be good enough, assuming the code works in the tests and on your actual files afterward.

@christianbrodbeck
Copy link
Member Author

@larsoner I could just copy-paste the filter settings from one of my files. I don't think it's ideal, since (as @cbrnr said) we don't know for sure whether the software filters also ought to affect the file in other places... so if someone has access to the original recording software maybe they could filter/export a data file to produce a real VHDR file? (@sappelhoff @hoechenberger @cbrnr ?)

@sappelhoff
Copy link
Member

so if someone has access to the original recording software maybe they could filter/export a data file to produce a real VHDR file?

I could and I would, but labs are closed down and I am not allowed to enter the institute. So soonest is probably in a couple of months.

Having that said, I am pretty sure that these settings do not affect other parts of the .VHDR file ... .VMRK is not affected for sure ... and .EEG is obviously only affected in that the data in there are actually filtered (but not the format, or setup of the file)

I agree with @larsoner that for now, copying suffices, without producing an entire new file (although I agree that would be the best practice).

@cbrnr
Copy link
Contributor

cbrnr commented Apr 20, 2021

I agree with @larsoner and @sappelhoff. Maybe we can create a real recording at a later point. For now, this is good to go.

@christianbrodbeck
Copy link
Member Author

Added – small update, I noticed there actually was a file with Software Filters, but it did not contain any channel names with spaces, and it was that combination that caused the error. The new test fails when reverting the fix commit.

Copy link
Member

@larsoner larsoner left a comment

Choose a reason for hiding this comment

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

LGTM +1 for merge

@larsoner larsoner merged commit 19b62e9 into mne-tools:main Apr 20, 2021
@larsoner
Copy link
Member

Thanks @christianbrodbeck !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants