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

Adding a reference does not work after a montage has been set #5619

Closed
cbrnr opened this issue Oct 17, 2018 · 11 comments · Fixed by #12160
Closed

Adding a reference does not work after a montage has been set #5619

cbrnr opened this issue Oct 17, 2018 · 11 comments · Fixed by #12160

Comments

@cbrnr
Copy link
Contributor

cbrnr commented Oct 17, 2018

The order in which you perform assigning a montage and adding a reference matters, which probably shouldn't be the case.

Consider the following example using the testing data from mne/io/brainvision/tests/data/test.* (the first two lines could be combined by supplying the montage argument in the call to mne.io.read_raw_brainvision):

raw = mne.io.read_raw_brainvision("test.vhdr", preload=True)
raw.set_montage("standard_1020")
raw = mne.io.add_reference_channels(raw, "T3")

This assigns the montage first and then adds a new reference channel which wasn't part of the data. This produces a RuntimeWarning: The locations of multiple reference channels are ignored (set to zero)., which results in an incorrect location of the newly added channel T3.

If instead you first add the reference channel and then set the montage, everything works as expected:

raw = mne.io.read_raw_brainvision("test.vhdr", preload=True)
raw = mne.io.add_reference_channels(raw, "T3")
raw.set_montage("standard_1020")
@agramfort
Copy link
Member

agramfort commented Oct 17, 2018 via email

@larsoner
Copy link
Member

The fix isn't obvious based on how applying montages currently works. One option is to add all montage entries to info['dig'], which isn't currently done. Then when adding a channel, check to see if it's in the list of digitized locations.

I'd probably just wait to tackle this once digitization/montages are properly refactored.

@cbrnr
Copy link
Contributor Author

cbrnr commented Oct 18, 2018

I didn't know that info['dig'] is needed in the referencing process, why is this necessary? I'm +1 on tackling this properly instead of a quick fix - is anyone working on this?

@larsoner
Copy link
Member

It's not needed for reference calculation. I assumed the problem you mention had to do with the location in space not being set. The error message might be misleading on this point, but I suspect it's the underlying problem.

Also I realized dig does not say which channel each point is associated with so the fix I proposed won't work. I can't think of a clean solution. We could hack something like storing the unused channel names and locations in case you add one later, but this is ugly and will not survive IO roundtrip.

I'm in inclined to say the solution is in the user side. Either use the other order with no warning, or live with the warning and reapply the montage afterward to get the location added. In other words, the rule is that applying a montage will only set locations of channels that exist at the time it is applied. And thus the fix here is just a documentation update to say that.

And nobody is actively working on the digitization refactoring yet. I hope to get to it for 0.18.

@cbrnr
Copy link
Contributor Author

cbrnr commented Oct 19, 2018

I agree that updating the documentation is the way to go here. I think a clear warning message would mention that you cannot add a new channel because a montage has already been set, and therefore the montage needs to be re-applied.

Alternatively, what about this hack: if the user adds a channel after a montage has been set, we could automatically re-apply the montage if we knew the montage name - is this stored anywhere? We could then only issue the warning if a channel that is not part of the montage was added (then of course we need to set its location to zero).

@agramfort
Copy link
Member

agramfort commented Oct 19, 2018 via email

@massich
Copy link
Contributor

massich commented Oct 19, 2018

is anyone working on this?

And nobody is actively working on the digitization refactoring yet. I hope to get to it for 0.18.

I don't mind teaming up with someone to approach this after we release. It would bring me light to a part of the io that is still shady to me.

@larsoner
Copy link
Member

@massich that would be awesome. Feel free to read the discussion in #3893, it contains the proposal.

@cbrnr
Copy link
Contributor Author

cbrnr commented Jun 6, 2019

FWIW, #6369 didn't solve this issue (maybe it wasn't supposed to, I didn't really follow this topic closely).

@agramfort
Copy link
Member

sure. #6369 aims to simplify the future changes by putting everything in one place. No real API change has been done yet.

@massich
Copy link
Contributor

massich commented Jun 7, 2019

I guess I should make an issue with the notes of the plan we discussed with @larsoner so that is easier to follow the status for everyone. Right now I only have messy notes.

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 a pull request may close this issue.

4 participants