Skip to content
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

[BUG] _channels_tsv writes default unit for channel type #81

Closed
sappelhoff opened this issue Sep 11, 2018 · 15 comments · Fixed by #125
Closed

[BUG] _channels_tsv writes default unit for channel type #81

sappelhoff opened this issue Sep 11, 2018 · 15 comments · Fixed by #125
Assignees
Labels
Milestone

Comments

@sappelhoff
Copy link
Member

mne_bids._channels_tsv writes a unit that is assumed based on the channel type:

units = [_unit2human.get(ch_i['unit'], 'n/a') for ch_i in raw.info['chs']]

This is problematic because different manufacturers save the raw data in different units. Sometimes there are even differences within single manufacturer data formats. Usually, the units are declared in the dataset - when reading the data with MNE-Python, the data gets automatically scaled to e.g., VOLTS so that all MNE-Python objects consistently work in VOLTS.

This is a problem, because we are then copying the non-mne-modified raw data to the bids directory ... and the units will not match.

@sappelhoff sappelhoff added the bug label Sep 11, 2018
@jasmainak
Copy link
Member

hmm ... so what are you proposing? Can you give an example where it fails?

@sappelhoff
Copy link
Member Author

sappelhoff commented Sep 11, 2018

For example the test_vhdr in #78 writes this channels.tsv:

Writing '/tmp/tmp_mne_tempdir_q1hu9plh/sub-01/ses-01/eeg/sub-01_ses-01_task-testing_run-01_channels.tsv'...

  name type units   ...   low_cutoff  high_cutoff  status
0  FP1  EEG     V   ...          0.0        250.0    good
1  FP2  EEG     V   ...          0.0        250.0    good
2   F3  EEG     V   ...          0.0        250.0    good
3   F4  EEG     V   ...          0.0        250.0    good
4   C3  EEG     V   ...          0.0        250.0    good

The units are V, however the data is actually in microVolts, see here:

https://github.com/mne-tools/mne-python/blob/5a84ba9f247362da66b209634916485d1c691c13/mne/io/brainvision/tests/data/test.vhdr#L18-L25

Currently just collecting all issues that I encounter while making #78 work. No concrete proposals ...

@jasmainak
Copy link
Member

okay sounds good! I'd recommend fixing it in a separate PR

@teonbrooks
Copy link
Member

it might be possible that this info is in the _raw_extras for the system specific io. I think this is mostly an eeg concern? maybe we can list the affected systems, and we can dive in for a solution

@sappelhoff
Copy link
Member Author

it might be possible that this info is in the _raw_extras for the system specific io

sounds interesting - how can that be accessed?

it's a problem for any system that does not natively save data in volts (so probably all EEG and iEEG data formats, don't know about MEG)

@sappelhoff
Copy link
Member Author

@teonbrooks can you answer to my question? If possible, I'd like to fix this bug before we release

@jasmainak
Copy link
Member

I fear you may have to dig into mne-python code for this ...

@sappelhoff
Copy link
Member Author

sappelhoff commented Oct 17, 2018

I don't have time to fix it before 0.1.0, so added it to 0.2.0 ... mne-tools/mne-python#5607 needs to be fixed first (see above)

@jasmainak
Copy link
Member

jasmainak commented Oct 26, 2018

@sappelhoff this would mean we should use the development version of mne, right? I guess it's unavoidable at some point since mne_bids is so tightly coupled to mne ...

@sappelhoff
Copy link
Member Author

I think that mne-tools/mne-python#5633 is ready to be merged, and then MNE will be released next week or so - so we can use MNE 0.17, right?

@jasmainak
Copy link
Member

I think @teonbrooks wanted to advertise MNE-BIDS at SFN, so I think ideally we'd like to have a release before that. And I don't think MNE is getting released before that ...

@sappelhoff
Copy link
Member Author

okay, fine by me to use master. I was always more inclined to do that - although the arguments against it are still valid ...

the plus side is, that it's making development easier for us. So, let's wait for the mentioned MNE-Python PR to be merged and then I'll open a PR to fix this present issue.

@agramfort
Copy link
Member

agramfort commented Oct 27, 2018 via email

@sappelhoff sappelhoff self-assigned this Oct 31, 2018
@sappelhoff
Copy link
Member Author

Now that mne-tools/mne-python#5633 was merged, I can start to work on this here :-)

@jasmainak
Copy link
Member

Perfect! May I suggest branching off from #106 so that you don't have a rebasing mess?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants