Closed sappelhoff closed 5 years ago
I think it would be nice to generate as many standard montage positions on the fly as possible rather than storing them in text files. We could basically change it so that they are calculated on the fly, and we only use the text file ones for validation (essentially they become test files, in principle you could git mv
them but it's probably not worth doing so).
What do you mean by git mv
ing them?
Other than that I completely agree with your point, however it seems like a major undertaking to implement this all at once.
Perhaps we could start by implementing the code to calculate the standard 10-20, 10-10, and 10-05 positions. Then whenever read_montage()
is called with e.g., kind='standard_1010'
, we would calculate the positions on the fly ... but when kind='biosemi16'
, we fall back on the files until someone implements an on-the-fly calculation for biosemi.
Another point is that for all positions that we calculate ourselves (as opposed to reading from a file) will not need to be rescaled into MNE standards --> which will solve the two important rescaling issues I referenced in the original post.
objection on my side. Can you open a PR to see what it would imply in terms of code change?
Loving the idea.
Please include SO1, SO2, M1, M2, A1, A2.
objection on my side
Any specific objections @agramfort ? Or a general reluctance?
Please include SO1, SO2, M1, M2, A1, A2
Do you have a documentation as to how these are defined with respect to the 1020/1010/1005 system @jona-sassenhagen ? LPA and RPA in my implementation are the same position as T9/T10 ... the two mastoids are very close to TP9 / TP10
Can you open a PR to see what it would imply in terms of code change?
In terms of code change, I thought we could start very slowly with the following schema:
read_montage
with kind
specified as e.g., standard_1005read_montage
we have an if/else clause that if the kind
is in list_of_positions_we_can_compute_instead_of_reading_from_file
with such a setup we can leave the code untouched for all positions that we cannot compute yet but it also allows a gradual transition to computing everything and not reading from files anymore.
One thing that I wouldn't know how to do is how to prevent MNE from rescaling the montages that we know are in the right format (because we computed them) ...
I meant "no objection". Sorry
@sappelhoff would it change code outside montage.py? any documentation? example?
@agramfort I did not start yet because I wanted to get a round of feedback before beginning. So feel free to suggest. And maybe @larsoner can help me with preventing the scaling.
I have reactions:
- if it's not broken don't fix it. So if there clear issues that one could
Benefit to me would be, I wouldn't have to think about where my loc files are stored anymore. This actually simplifies things a lot, because it removes context dependence: I can simply tell everyone that to get channel locations, they should execute this line, regardless of their specific 10/20 setup.
- if not, can it allow to remove a lot of lines ie a PR will mostly red lines which would be a win in future maintenance.
In principle, we could drop a lot of the loc files we have, which would in fact be 100s of lines of code. But we should not remove API, cause this works only for standard montages and people need to be able to set up their nonstandard ones.
I don't quite see the advantage of this either. Why don't we just include the 10/5 montage file with all 345 channels? We only need this file to get all possible subsets of 10/5 montages, including 10/20. Having the actual calculation is then not necessary, because I don't think you can cram in more than these 345 electrodes anyway.
We have multiple loc files right now - a whole directory of them. We need a few lines for testing, but many of the loc files can be simply dropped.
What I would really like is if we could have, in the raw constructor, an option to go montage=“10/20”, and it would automatically fill in channel names. That would really simplify reading in standard EEG files for newcomers.
From: Clemens Brunner notifications@github.com Sent: Monday, June 11, 2018 9:48:51 AM To: mne-tools/mne-python Cc: jona-sassenhagen; Mention Subject: Re: [mne-tools/mne-python] Feature: Standard electrode position computation (#5269)
I don't quite see the advantage of this either. Why don't we just include the 10/5 montage file with all 345 channels? We only need this file to get all possible subsets of 10/5 montages, including 10/20. Having the actual calculation is then not necessary, because I don't think you can cram in more than these 345 electrodes anyway.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/mne-tools/mne-python/issues/5269#issuecomment-396153040, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AEHyImwwT8bkrgbllNbImFqd2uFm57eTks5t7iDjgaJpZM4UeJqi.
We can replace all montage files that are a subset of 10/05 or 10/20 with one file containing the 345 labels and locations of the 10/05 system. Passing a montage as string in the constructor already works, so all we need is the file. Also, currently there are only 2 files with 10/20 montages, so I'm not sure if any other montage file could be dropped...
We can replace all montage files that are a subset of 10/05 or 10/20 with one file containing the 345 labels and locations of the 10/05 system. Passing a montage as string in the constructor already works, so all we need is the file.
If people can just write "10/20" there, I think that would be very beginner friendly.
Also, currently there are only 2 files with 10/20 montages, so I'm not sure if any other montage file could be dropped...
There's many more, e.g., all of the standard_* ones, and one of the easycap ones.
If people can just write "10/20" there, I think that would be very beginner friendly.
"10/20" is not a valid file name, but we could create a montage file called "10-20" or "10-05".
There's many more, e.g., all of the standard_* ones, and one of the easycap ones.
But they all contain different labels, so the mapping between the various names has to be stored somewhere. I suggested to unify these montages at some point, but I forget what we decided.
In summary, calculating the positions or having one montage file containing all possible locations are two equivalent ways to solve this problem. What we need to to in addition is to create mappings between various labels that have identical positions.
Although I think having the calculation would be quite nice, a large montage file would also get the job done (and no new code needs to be added). In both cases, we need the labels mapping part.
I don't understand the label mapping part - what is the problem exactly?
We only have multiple 10/20 montage files because each contains different labels for the same positions (and each file contains a different subset of the 345 possible 10/05 locations). For example, standard_primed
has a label F7'
, whereas the same location is called F7a
in standard_postfixed
.
Yes, but can't we just drop the superset in one file?
That's basically what I'm saying. But currently, we can't do that because we would have to either invent our own file format that supports multiple labels for one position or properly detect two or more identical locations (right now this is a problem when plotting the montage, because multiple identical positions get overplotted).
In summary, calculating the positions or having one montage file containing all possible locations are two equivalent ways to solve this problem. What we need to to in addition is to create mappings between various labels that have identical positions.
Although I think having the calculation would be quite nice, a large montage file would also get the job done (and no new code needs to be added). In both cases, we need the labels mapping part.
I agree with what @cbrnr says here. Although that I still think having the code instead of a file would be worth it because it is more transparent where the values come from.
That's basically what I'm saying. But currently, we can't do that because we would have to either invent our own file format that supports multiple labels for one position or properly detect two or more identical locations (right now this is a problem when plotting the montage, because multiple identical positions get overplotted).
We can just map from names to labels? As long as no two labels occupying the same position actually show up in the EEG file, I don't see the problem.
Yes @sappelhoff, I think having the code is really useful to reproduce the locations. I was just arguing that an alternative to adding about 650 lines of additional code to MNE would be a unified location file.
@jona-sassenhagen I see that I already fixed the plotting problem in #4592, so this shouldn't be an issue anymore.
Ah I think I get now what I missed before.
Still, I'd think it'd be cool if people could do read_raw_xxx(fname, montage="10/20")
.
Sure, we could special-case the string "10/20"
. IMO the quickest solution would be to go ahead and create a new 10/20 montage file with all possible 300+ locations, which we can refer to as "10/20"
with the montage
argument. Ideally, these locations would be the output of @sappelhoff's routines. Ideally, there shouldn't be duplicate names in this montage, but then we'd need a way to map multiple possible channel names to one location. For example, a simple dict might be sufficient here, e.g. {"C3": [], "C3a": ["C3'", "C3."], ...}
(I'm making up the mappings here, but the point is that the keys correspond to the 300+ unique labels in the montage, and the values are lists with possible duplicate names).
PS: Just to make sure, you do know that read_raw_xxx(fname, montage="standard_1005")
already works, right? The argument needs to be an existing montage file name.
Could we maybe add a log message when reading in raw files without a montage, and if the channel names appear to be 10/20 (could be easily checked), that the user could do as @cbrnr suggested above if they want to use channel locs?
I'm mainly thinking about making life easier for newbie EEG people. Not being required to figure out how montages work and where the montage files are placed/bring their own montage files, would be helpful.
@agramfort
what would this imply in terms of code / API change?
Nothing :) Just a log message if:
let's see how this PR looks like :)
I'll do it if some of the other EEG people think it might be a good idea ... @cbrnr @mmagnuski @sappelhoff ?
Sure, I like it!
@cbrnr ? Would like to have your opinion first.
Uh, @cbrnr , another issue, maybe I misunderstand you, but ... this doesn't actually work.
import mne
path_eeglab = mne.datasets.testing.data_path()
fname = path_eeglab + '/EEGLAB/test_raw.set'
event_id = {"rt": 1, "square": 2}
raw = mne.io.read_raw_eeglab(fname, event_id=event_id, preload=True, montage="standard_1005")
@jona-sassenhagen mne.channels.read_montage("standard_1005")
works, and passing montage along with read_raw_eeglab
seems to be passing some specific path to read_montage
(probably assuming it is a file in the same directory as the file you are reading in?) which leads to file not found error.
Would be nice if it worked.
Yes. As-is, it is needlessly complicated in workshop situations where people bring their own files.
Suggestion: the basic io functions should search not only locally, but also mne/channels/data/montages/
.
hmm ... it should work just like the EGI reader or EDF reader. If it doesn't, perhaps there is something to fix
I am open to suggestions if you report that people struggle to load their data.
can give you an example of file and code that is unnecessarily complicated?
Not struggle - it would just mean, EEG people would not have to think about montages and layouts, but could just load their raw data with montage=standard_1005
and it would "just work".
you mean passing a string to montage that would work as read_montage?
Hm ... ok, I think I see the problem.
This works:
mne.io.read_raw_brainvision(fname, montage="standard_1005")
but this doesn't:
mne.io.read_raw_eeglab(fname, montage="standard_1005")
gives
FileNotFoundError: [Errno 2] No such file or directory: ''
So:
fix montages from the standard folder for eeglab reader
give a log message when people read in EEG files with only 10/20 channel names and no montage that they can just give "standard_1005" as their montage and they will get the channel locations
I like the idea to make things easier for newbies. I think specifying the name of a montage as a string in mne.channels.read_montage
was a step in the right direction, but this should be extended for all mne.io.read_raw_*
functions, so I'd modify your first bullet point to "check all readers if they accept strings for their montage
argument".
Giving log messages is great, let's not do anything automatically here. However, maybe this message can also be triggered if not all channel names are found in standard_1005
, but only some names match?
+1 for fix read_raw_eeglab and check all EEG data readers.
Giving log messages is great, let's not do anything automatically here.
However, maybe this message can also be triggered if not all channel names are found in standard_1005, but only some names match?
no opinion
@sappelhoff ok if we repurpose the issue for that ..?
@jona-sassenhagen yes, it's a good improvement and I can reopen this issue with its original purpose at some other time. Before doing more intense work on the way we handle our coordinates, the crucial #5190 should be fixed through #5085.
@sappelhoff how about for now we add a link in the montage
code somewhere to the separate repo? That way if we hit a use case where we need to calculate, we can refer to that?
sounds good, perhaps link to it in some of the documentation pages? Or did you mean as a comment somewhere in the code?
Either (or both) is fine by me
looping in @drammock who is working hard on the docs: Would you know a good place in the documentation where we could refer to a repository for computing standard EEG electrode positions on a sphere? (talking about: https://github.com/sappelhoff/eeg_positions) ... perhaps reading the OP will aso provide you with the background for responding to this,
Possible locations for adding the link with a brief (2 sentences) description:
Similar pages that we have:
Okay, a minimal note with a link is added to the docs. It'll be merged with https://github.com/mne-tools/mne-python/pull/5944
I have recently finished a little pet project of mine, in which I compute all 345 standard EEG electrode positions of the 10-05 (5%) system. Although that's nothing new per se and the data are available e.g., from Robert Oostenveld, I thought that it would be nice for MNE to have the actual computations, which then yield the data to be used as part of the code. Currently, there are only a bunch of data files in
mne/channels/data
which is not as transparent as the code would be.... as I started to hint at on gitter yesterday, this also relates to the issues about montages, layouts, and digitization, see #5190 #3987
What do you think?
This is the repository containing the computations I am talking about.
Here is a short notebook showing an initial integration with MNE.
And a plot: