mne-tools / mne-python

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

Unused setup_volume_source_space parameters. #3708

Closed jaeilepp closed 7 years ago

jaeilepp commented 7 years ago

setup_volume_source_space has three params that it is not using subject, subjects_dir and overwrite.

larsoner commented 7 years ago

fname is also a silly argument, since we have write_source_spaces.

I have also never liked that we have setup_source_space and setup_volume_source_space, the names are not consistent. I wonder if we should take this opportunity to make new functions without the old API cruft. Eventually we could deprecate the old ones, but they could just as easily stick around as wrappers with clear .. note::s in the docstring suggesting use of the new functions.

agramfort commented 7 years ago

what API do you suggest?

larsoner commented 7 years ago

Thinking out loud after some git grep work, generally use make_ instead of setup_ in our function names, so we could start there:

make_surface_source_space
make_volume_source_space
make_discrete_source_space

More explicit, more consistent. And we can shed old, unused, or mis-named arguments.

agramfort commented 7 years ago

@dengemann any concern?

dengemann commented 7 years ago

Ok by me, I somehow implicitly felt that "make" is about impure functions that write stuff to disk but it is not consistently the case. Fine to change names if the overall picture is more consistent. Although I hate the resulting deprecation cycle. Let's assume MNE is used in a few years by 10 times as many people. They will be happy.

agramfort commented 7 years ago

fine with me then.

allow writing in these make_ functions is also something we should avoid.

we could try however to a default so :

write_source_space(src)

works out of the box.

larsoner commented 7 years ago

Agreed, the writing should be separate. Within those functions IIRC all they do is call write_source_spaces under the hood anyway

agramfort commented 7 years ago

are you on it?

larsoner commented 7 years ago

not right now, so feel free if you have time

otherwise, yes I can probably do it this week

larsoner commented 7 years ago

...actually come to think of it, we probably should use subject and subjects_dir, along with mri, to construct the MRI volume to use if possible. In other words, a user should just be able to say setup_volume_source_space('sample', mri='T1.mgz', subjects_dir=subjects_dir) and have it work. And we should have it use overwrite, that is a bug. So I think instead of this deprecation stuff we should actually have it use the parameters properly / more usefully.