Add support for importing channel location from XYZ/CSV files. #9203

jackz314 · 2021-03-27T06:04:22Z

Reference issue

"Spin-off" from #9192.

What does this implement/fix?

Add support for importing channel location from XYZ/CSV files as an extra XYZ option (to be finalized) for read_custom_montage as per suggestion from here.

Additional information

The naming (XYZ?) & formatting of this feature isn't fully finalized yet, I'm still waiting for input from more qualified people on that.

…han_loc

agramfort

tell us when you added test and a what's new entry and it's good to go from your end.

jackz314 · 2021-03-31T09:32:50Z

I think feature-wise it's done, but I'm not sure what's good enough for the tests. Should I just import from a CSV and compare the resulting montage to the CSV data? That's how I "tested" it locally, but I'm not sure if it's good and comprehensive enough.

If that's the case then I think the existing test_montage_readers function might be good enough, all I need to do would be add some fake CSV data and expected digs/montages.

agramfort

do you need help to add some unit tests?

agramfort · 2021-03-31T10:12:58Z

mne/channels/_standard_montage_utils.py

+
+
+def _read_csv(fname, delimiter=','):
+    """Import eeg channel locations from CSV files into MNE instance.


Suggested change

"""Import eeg channel locations from CSV files into MNE instance.

"""Import EEG channel locations from CSV files into MNE instance.

agramfort · 2021-03-31T10:13:27Z

mne/channels/_standard_montage_utils.py

+    delimiter : str
+        Delimiter used by the CSV file
+    include_ch_names : bool
+        Whether the CSV file include channel names as the first column


this parameter does not exist

Ah, I forgot to remove this after I removed the actual parameter. Would you say it's beneficial to add these parameters (delimiter and ch_names) to read_custom_montage so the CSV files can be more flexible? Or should I just remove both of them?

jackz314 · 2021-03-31T11:46:43Z

do you need help to add some unit tests?

Some help would be great since I'm still not really familiar with the MNE testing standards.

drammock · 2021-03-31T15:09:07Z

mne/channels/_standard_montage_utils.py

+        Whether the CSV file include channel names as the first column
+    """
+    include_ch_names = True
+    import csv


python standard library imports should go top-of-file, not nested inside function. We only nest optional dependencies (or internal imports to avoid circular import errors)

drammock · 2021-03-31T15:09:40Z

mne/channels/_standard_montage_utils.py

+    # chs = inst.info['chs']
+    # if not include_ch_names:
+    #     for ch, coord in zip(chs, coords):
+    #         ch['loc'][:3] = coord
+    # else:
+    #     for ch in chs:
+    #         try:
+    #             coord = coords[ch_names.index(ch['ch_name'])]
+    #         except ValueError:  # ch_name not in channel list, default to 0
+    #             coord = (0, 0, 0)
+    #         ch['loc'][:3] = coord


drammock · 2021-03-31T15:11:36Z

mne/channels/_standard_montage_utils.py

+    include_ch_names : bool
+        Whether the CSV file include channel names as the first column
+    """
+    include_ch_names = True


this is now always true. remove it and eliminate the conditionals that depend on it.

drammock · 2021-03-31T15:13:00Z

mne/channels/_standard_montage_utils.py

+    f = open(fname, "r")
+    f.readline()
+    ch_names = []
+    pos = []
+    for row in csv.reader(f, delimiter=delimiter):
+        if include_ch_names:
+            ch_name, x, y, z, *_ = row
+            ch_names.append(ch_name)
+        else:
+            x, y, z, *_ = row
+        pos.append((float(x), float(y), float(z)))
+    f.close()


we typically use the with open(fname, 'r') as fid: context manager, so we don't have to expressly close the file afterward. For consistency we should do the same here.

drammock · 2021-03-31T15:18:27Z

mne/channels/_standard_montage_utils.py

+    CSV files should have columns x, y, and z, each row represents one channel.
+    Optionally the first column can contain the channel names.


the way the function is now written, channel names as first column are not in fact optional. Revise this text to reflect that.

drammock · 2021-03-31T15:20:04Z

mne/channels/_standard_montage_utils.py

+            ch_names.append(ch_name)
+        else:
+            x, y, z, *_ = row
+        pos.append((float(x), float(y), float(z)))


(nitpick) you could omit the three float() calls here, and just add dtype=float when you later convert pos to a numpy array. Seems cleaner to me that way.

drammock · 2021-03-31T15:24:47Z

mne/channels/_standard_montage_utils.py

    return make_dig_montage(ch_pos=_check_dupes_odict(ch_names, pos))
+
+
+def _read_csv(fname, delimiter=','):


not clear to me why delimiter is exposed here. It's a private function (so users can't actually change it) and it's only called once in the codebase and there's no triaging based on file extension (csv vs tsv). Do you know there to be XYZ channel location text files that use a different delimiter? If so, we should expose delimiter in the public API somehow. If not, we should assume comma-delimiter and remove this param.

delimiter is a leftover parameter, the only different ones are probably tabs in TSV files. Should this be checked in read_custom_montage or in _read_csv? As for the channel names, I've seen files with channel names (mainly CSV files) and ones without (mainly XYZ files).

agramfort · 2021-03-31T16:30:47Z

pushed a commit to add a test and address some of the comments

…

drammock

just 2 small docstring fixes.

drammock · 2021-03-31T17:00:27Z

mne/channels/_standard_montage_utils.py

+
+    CSV files should have columns 4 columns containing
+    ch_name, x, y, and z. Each row represents one channel.
+    The first column can contain the channel names.


Suggested change

The first column can contain the channel names.

drammock · 2021-03-31T17:00:52Z

mne/channels/_standard_montage_utils.py

+    Parameters
+    ----------
+    fname : str
+        Name of the csv file to read channel locations from


Suggested change

Name of the csv file to read channel locations from

Name of the csv file to read channel locations from.

agramfort · 2021-03-31T17:15:57Z

@jackz314 do you want to take over?

jackz314 · 2021-03-31T21:23:08Z

I'm finalizing the changes now. Should we change _read_csv to _read_xyz? Since it should handle all three formats (csv, tsv, and xyz)? Also, it seems like the norm for XYZ files is that channel names are in the last column, but I've also seen some, where it's the first column, what should we do about this?

agramfort · 2021-03-31T21:40:36Z

Csv is not a standard. We cannot do magic read. With what you say I am now tempted to say user should just call make dig montage and deal with reading the csv on their side. We cannot claim to support more. Take home message : avoid black magic as it will backfire

jackz314 · 2021-03-31T22:04:09Z

I pushed a commit with some code to adapt for TSV, CSV, and XYZ files. Since CSV isn't a standard, I just specified and used the ch_name, x, y, z format, which follows the TSV standard used elsewhere. For XYZ files I used the EEGLAB standard (or just the standard EEGLAB uses), which is row_count, x, y, z, ch_name.

agramfort · 2021-04-01T07:18:44Z

@jackz314 I added some tests. See my commit

To run the tests you can use:

pytest mne/channels/tests/test_montage.py

jackz314 · 2021-04-01T09:04:55Z

Thanks for the tests, I ran them locally and it seems like they are passing. I think this should be ready unless there's anything else needed.

agramfort

@jackz314 you need now to add a what's new entry in the file latest.inc in the doc folder

also I feel that we should update the page https://mne.tools/dev/auto_tutorials/intro/plot_40_sensor_locations.html

@drammock you confirm it would be the right place?

drammock · 2021-04-01T14:19:04Z

also I feel that we should update the page https://mne.tools/dev/auto_tutorials/intro/plot_40_sensor_locations.html
@drammock you confirm it would be the right place?

Not sure I agree. read_custom_montage isn't really covered anywhere in the tutorials (it is used once in passing in https://mne.tools/dev/auto_tutorials/source-modeling/plot_eeg_mri_coords.html). I think it might make more sense to create a new how-to example: "how to load a custom EEG sensor montage" in the input/output category.

…han_loc

jackz314 · 2021-04-01T16:41:57Z

Just added the what's new entry.

I think it would make a lot of sense to add read_custom_montage to a tutorial somewhere since otherwise, it might be confusing for people just learning about MNE. I think it would make sense to add it to the existing page https://mne.tools/dev/auto_tutorials/intro/plot_40_sensor_locations.html since it's titled "Working with sensor locations", which seems like a natural place for adding/reading custom sensor locations.

drammock · 2021-04-01T16:58:31Z

Just added the what's new entry.

I think it would make a lot of sense to add read_custom_montage to a tutorial somewhere since otherwise, it might be confusing for people just learning about MNE. I think it would make sense to add it to the existing page https://mne.tools/dev/auto_tutorials/intro/plot_40_sensor_locations.html since it's titled "Working with sensor locations", which seems like a natural place for adding/reading custom sensor locations.

I guess it would be OK to add a section between "working with built-in montages" and "controlling channel projection" called "working with custom montages" that shows a quick example of read_custom_montage. It should probably list the various formats that we support (not just XYZ).

doc/changes/latest.inc

agramfort

anyone merge if happy

thx @jackz314

drammock · 2021-04-02T21:58:22Z

@jackz314 are you planning to add to the tutorial in this PR, or in a separate one?

jackz314 · 2021-04-03T02:15:36Z

@jackz314 are you planning to add to the tutorial in this PR, or in a separate one?

I wasn't planning on adding the tutorials myself, mostly because I'm not really familiar with how the tutorial part or the other kinds of montages work yet. But if no one else is working on it, I can look into it and open another PR later if I have time.

drammock · 2021-04-03T02:46:10Z

Ok I'll merge this one now then.

welcome · 2021-04-03T02:46:32Z

🎉 Congrats on merging your first pull request! 🥳 Looking forward to seeing more from you in the future! 💪

jackz314 added 2 commits March 25, 2021 02:00

Add xyz (csv) support to read_custom_montage.

3c884aa

Merge branch 'main' of https://github.com/mne-tools/mne-python into c…

2a1de95

…han_loc

agramfort reviewed Mar 27, 2021

View reviewed changes

Fix _read_csv bugs.

e5493a5

agramfort reviewed Mar 31, 2021

View reviewed changes

drammock requested changes Mar 31, 2021

View reviewed changes

add test + cleanup"

72320c0

drammock approved these changes Mar 31, 2021

View reviewed changes

jackz314 added 2 commits March 31, 2021 14:58

Adapt for csv, tsv, and xyz.

5ea0991

Fix xyz read.

156c677

jackz314 and others added 2 commits March 31, 2021 15:16

Fix comment style.

0692782

add tests

1059fa1

jackz314 marked this pull request as ready for review April 1, 2021 09:04

agramfort reviewed Apr 1, 2021

View reviewed changes

jackz314 added 2 commits April 1, 2021 09:27

Add what's new entry for _read_xyz.

8d023d7

Merge branch 'main' of https://github.com/mne-tools/mne-python into c…

6e957aa

…han_loc

Fix changelog typo.

c36a61b

agramfort reviewed Apr 1, 2021

View reviewed changes

doc/changes/latest.inc Outdated Show resolved Hide resolved

doc/changes/latest.inc Show resolved Hide resolved

agramfort reviewed Apr 1, 2021

View reviewed changes

doc/changes/latest.inc Outdated Show resolved Hide resolved

Fix changelog typo.

73d7ee5

agramfort approved these changes Apr 1, 2021

View reviewed changes

drammock merged commit 2897d4b into mne-tools:main Apr 3, 2021

jackz314 deleted the chan_loc branch April 3, 2021 17:37



		def _read_csv(fname, delimiter=','):
		"""Import eeg channel locations from CSV files into MNE instance.

		CSV files should have columns x, y, and z, each row represents one channel.
		Optionally the first column can contain the channel names.

		return make_dig_montage(ch_pos=_check_dupes_odict(ch_names, pos))


		def _read_csv(fname, delimiter=','):

	Name of the csv file to read channel locations from
	Name of the csv file to read channel locations from.

Uh oh!

Add support for importing channel location from XYZ/CSV files. #9203

Add support for importing channel location from XYZ/CSV files. #9203

Uh oh!

Conversation

jackz314 commented Mar 27, 2021

Reference issue

What does this implement/fix?

Additional information

Uh oh!

agramfort left a comment

Choose a reason for hiding this comment

Uh oh!

jackz314 commented Mar 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

agramfort left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jackz314 commented Mar 31, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agramfort commented Mar 31, 2021 via email

Uh oh!

drammock left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agramfort commented Mar 31, 2021

Uh oh!

jackz314 commented Mar 31, 2021

Uh oh!

agramfort commented Mar 31, 2021 via email

Uh oh!

jackz314 commented Mar 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

agramfort commented Apr 1, 2021

Uh oh!

jackz314 commented Apr 1, 2021

Uh oh!

agramfort left a comment

Choose a reason for hiding this comment

Uh oh!

drammock commented Apr 1, 2021

Uh oh!

jackz314 commented Apr 1, 2021

Uh oh!

drammock commented Apr 1, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

agramfort left a comment

jackz314 commented Mar 31, 2021 •

edited

Loading

jackz314 commented Mar 31, 2021 •

edited

Loading