sappelhoff commented 2 years ago

see https://github.com/mne-tools/mne-python/issues/10678#issuecomment-1139686322 and following 4 comments

In brief:

the idea is to have one "export" function (and method) for raw, epochs, and evoked
these functions delegate all the heavy lifting to functions that ideally are maintained and developed outside of mne (such as https://github.com/bids-standard/pybv) ... mne mostly contains a small wrapper around the external heavy lifting functions
in some cases, these wrappers will be more involved (see EDF e.g., https://github.com/mne-tools/mne-python/blob/main/mne/export/_edf.py)
... and in some cases the "heavy lifting" function might be internal altogether (see MFF e.g., https://github.com/mne-tools/mne-python/blob/main/mne/export/_egimff.py)
ideally though, the code in mne is minimal.

the problem

some exporting funcs will accept additional parameters that are irrelevant to other exporting funcs
for EDF this is done by adding these params to the general export function and noting only for EDF, see: https://mne.tools/stable/generated/mne.export.export_raw.html#mne.export.export_raw
yet for MFF, the solution is to make the heavy lifting export function itself public, see: https://mne.tools/stable/generated/mne.export.export_evokeds_mff.html

this is inconsistent, and we should clean that up / make a consistent choice before the module continues to grow.

IMHO, the current "MFF way" is not a good style and would lead to lots of confusing public functions, instead of the clean "trinity" of raw, epochs, evoked .export. Couldn't we solve this by allowing **kwargs and documenting the kwargs that can be passed? Similar to how we do it in ICA when users pick picard or fastica, etc.

What are your opinions?

cbrnr commented 2 years ago

+1 for **kwargs.

agramfort commented 2 years ago

ok for me.

Message ID: @.***>

larsoner commented 2 years ago

Agreed, let's go with **kwargs. I think there is no need to deprecate export_evokeds_mff, we can just make it so that export_evoked takes over the functionality (and people can transition to it easily/whenever) and we can remove it from the API docs.

larsoner commented 1 year ago

I think we have implemented the export interfaces so I'll close, but feel free to reopen if I missed something @sappelhoff

larsoner commented 1 year ago

Argh actually I think the inconsistency is still there, reopening. Sorry for the noise!

mne-tools / mne-python

Export module inconsistent, needs API decision #10682

the problem