-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Axial to planar gradiometer transformation #13196
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
base: main
Are you sure you want to change the base?
Conversation
This is based on the channel positions and orientations provided by fieldtrip: https://github.com/fieldtrip/fieldtrip/blob/master/template/gradiometer/ctf275.mat
This is based on the channel positions and orientations provided by fieldtrip: https://github.com/fieldtrip/fieldtrip/blob/master/template/gradiometer/neuromag306.mat
|
Hello! 👋 Thanks for opening your first pull request here! ❤️ We will try to get back to you soon. 🚴 |
That way we can parse the txt files and select the channel type we want to interpolate to. For example we do not want to interpolate to the ch_type = 'ref' for the CTF.
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
|
Looks like you're making some progress, let me know when you'd like some feedback! |
|
Hi @larsoner! After a while, I finally found some time :) I think this is a good moment for some feedback. Issues / Questions I encountered: My initial understanding was that when we interpolate from CTF to Neuromag, the ch_type should already be known. This is because, CTF systems have reference sensors that should not be interpolated and Neuromag systems have gradiometers and magnetometers, so we need to know the type of each channel. That's why I added the ch_type parameter in the .txt montage files — to explicitly specify this. 2. Scope of interpolation: only gradiometers? Based on this, I thought I would input in 3. Limitations of make_dig_montage From the documentation of I understood that for my setup (no fiducials) I should use a custom montage. However: running 4. Custom _meg() function and related problems But now I run into new problems. When I run
5. I added _meg() inside _standard_montage_utils but am I allowed to change such private function ? 6. I am trying to follow how interpolate_to() code style and structure is already built. However, this forces us to diverge a bit from the standard One idea I had would be to encapsulate some logic in a new function |
Added new montage data files for CTF151, CTF275, and Neuromag306 systems in CSV format to mne/channels/data/montages/. These files provide sensor location and orientation information.
Introduces the read_meg_montage function to load canonical MEG sensor positions and orientations from CSV files for supported systems ('neuromag', 'ctf151', 'ctf275'). This utility constructs an Info object with sensor metadata for use in field interpolation.
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
|
@larsoner, @britta-wstnr this pr is ready for review. :) The interpolation from Neuromag to CTF works! (see the example script I uploaded). Quite some github actions seem to fail, let me know if I can fix something to let them pass. Notes:
|
At least as a first step I'll merge |
* upstream/main: MAINT: Auth [skip azp] [skip actions] MAINT: Deploy [circle deploy] [skip azp] [skip actions] Bump github/codeql-action from 3 to 4 in the actions group (mne-tools#13442) ENH: Dont constrain fiducial clicks to mesh vertices (mne-tools#13445) Use timezone-aware ISO 8601 for website timestamp (mne-tools#13347) [pre-commit.ci] pre-commit autoupdate (mne-tools#13443) FIX: Update osf.io links to new format (mne-tools#13440)
I think ideally here you would modify some example, and some tests. For a simple test, if you take Neuromag data, transform it to CTF, and back to the original Neuromag info / dev_head_t, are the start and end data very highly correlated? I would also check that the montages loaded for Neuromag match the For failed tests you should be able to click on the name of any failed CI. So for example "ubuntu-latest pip" which I know runs almost all the tests, you can see a few failures: |
I will add tests for that
Do you have a similar test for EEG montages, just to get some inspiration? I generated all the MEG montages directly from the .info component after loading the dataset in mne, so every MEG montage and .info contain identical sensor positions. The Neuromag306 and CTF151 datasets I used come from the MNE-Python sample data, so I trust their .info fields to be accurate. For the CTF275, I performed a small visual check to verify that the dataset I received from UCL/SPM has the correct sensor positions for the CTF275 system. I compared the CTF275 data from the UCL/SPM dataset with a Donders dataset I have, and the gradiometer positions differed only by about a millimeter. |
This comment was marked as resolved.
This comment was marked as resolved.
|
I had forgotten some old code in this PR. I reverted all the commits that had this code. Now let's wait and see if the tests will pass ;) |
Reorganized the code to separate EEG and MEG system transformation sections.
| # TODO: Refactor not to use Pandas (should be able to parse without it) | ||
| df = pd.read_csv(csv_file) |
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.
pandas is an optional dependency of MNE and not everyone will have it. Is it easy enough to just parse the CSV directly / manually? Usually we try to do that if it's not too much work
| if is_eeg_interpolation: | ||
| # Check that the method option is valid. | ||
| _validate_type(sensors, DigMontage, "sensors") |
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.
The changes here are mostly whitespace and nest things in conditionals. Rather than doing this, would it be possible to do something like
def interpolate_to(...)
...
if is_eeg_interpolation:
self._interpolate_to_eeg(...) # which contains most of the old code, unmodified
if is_meg_interpolation:
self._interpolate_to_meg(...) # which contains most of the new code
This separates out the functionality a bit in a more readable way, and improves git blame
Reference issue (if any)
Fixes #9609 .
What does this implement/fix?
Additional information
The channel positions and orientations are adopted from fieldtrip: https://github.com/fieldtrip/fieldtrip/blob/master/template/gradiometer
I am not sure if for the interpolation we need the coil positions and orientations. For clarity: a gradiometer is ONE channel but has TWO coils. A magnetometer is ONE channel and has ONE coil.
As an expectation I have that I will plot ERPs in the ctf and neuromag format. Also I want to create topoplots like: https://www.fieldtriptoolbox.org/assets/img/tutorial/eventrelatedaveraging/figure8.png