search

mne-tools / mne-python

MNE: Magnetoencephalography (MEG) and Electroencephalography (EEG) in Python
https://mne.tools
BSD 3-Clause "New" or "Revised" License
2.7k stars 1.31k forks source link

Add fiducial point check in _read_xyz #9510

Open sans-dev opened 3 years ago

sans-dev commented 3 years ago

Hi everyone. Would it be a good idea to add a check for fiducial points in _read_xyz function? Right now I am dealing with a custom montage in .csv format where nasion, lpa and rpa coordiantes are written into the csv. By checking for some standard namings ('nasion', 'nz', 'lpa', 'rpa') inch_pos we could extract their coordinates and pass them into make_dig_montage.

https://github.com/mne-tools/mne-python/blob/3078533f8704820a168797578f5b47ddff4be347/mne/channels/_standard_montage_utils.py#L345

welcome[bot] commented 3 years ago

Hello! 👋 Thanks for opening your first issue here! ❤️ We will try to get back to you soon. 🚴🏽‍♂️

agramfort commented 3 years ago

are you relying on a private function for your script?

sans-dev commented 3 years ago

No, I am calling read_custom_montage, which then internally uses the _read_xyz function based on the file extension (csv in this case).

agramfort commented 3 years ago

we could expose some extra parameters in read_custom_montage for this.

I am reluctant to make too much magic

sans-dev commented 3 years ago

Parametrization is a good idea. I suggest a list of string labels. 

_read_theta_phi_in_degrees(fname, head_size, fid_names=None,
                               add_fiducials=False):

_read_theta_phi_in_degrees already provides such an interface. We could transfer it to _read_xyz. The initialization would happen when read_custom_montage is called. We could use default arguments there.

agramfort commented 3 years ago

I would add new parameters to read_custom_montage

what we do in functions that start with _ is not a problem. It's private so we can do whatever we want there. What matters is the changes to read_custom_montage