MRG: Add argument overwrite to export for raws and epochs to match save.

[mscheltienne]

I have to do a couple of I/O roundtrips to EEGLAB, and I could use an overwrite argument, set to False by default, for the export methods as in inst.save to prevent unintentional exports.

For inst.save, when overwrite was added, it was first initialized to True for one version and then changed to False.

This defaults to True in 0.18 but will change to False in 0.19.
New in version 0.18.

If you agree with the addition of this argument, do you want to follow the same path?

[larsoner]

Agreed we should have it. However, the export stuff was added in 0.24 so I think we can just backport the addition of overwrite as a bugfix here to make our lives easier

[larsoner]

(i.e., just have overwrite=False and don't bother with any deprecation cycle)

[agramfort]

good idea !

[mscheltienne]

Quick question while the CI is running, if you intend to backport this to 0.24 and to avoid the deprecation cycle, do you want to have .. versionadded:: 1.0 or .. versionadded:: 0.24?

[larsoner]

.. versionadded:: 0.24.1 would be best in this case I think

[mscheltienne]

Ok, one more question: when I look at the documentation for the added export module, I see:

Why is mne.export.export_evokeds_mff the only public function out of the export functions? Shouldn't it be private as the export function for Raw/Epochs towards EEGLAB or EDF? It's covered by mne.export.export_evokeds and it's complicating the API without any gain IMO.

Also, I will add overwrite to mne.export.export_evokeds as well.

[larsoner]

Why is mne.export.export_evokeds_mff the only public function out of the export functions?

It allows for extra arguments not usually needed or used by the other exporters. See #9406 (comment) and other points of discussion in that PR if interested

[larsoner]

You can ignore pip-pre for now as it should be fixed by #9973

[mscheltienne]

Ok, it makes sense. Personally, I would have been in favor of passing **kwargs to the exporter + good documentation/check of the possible argument in kwargs per exporter rather than multiplying the public functions, but I see the point with the nasty parameter list.

I will add overwrite to both evoked exporters then.

[mscheltienne]

@larsoner Well adding overwrite for evoked is not as simple.. I think it would be best for MNE API to be consistent and to have this parameter everywhere, but mffpy doesn't support it,

git\mne-python\mne\export\_egimff.py:56: in export_evokeds_mff
    writer = mffpy.Writer(fname)
pyvenv\mne\lib\site-packages\mffpy\writer.py:34: in __init__
    self.filename = filename
pyvenv\mne\lib\site-packages\mffpy\writer.py:127: in filename
    assert not exists(fn), f"File '{fn}' exists already"
E   AssertionError: File 'C:\Users\Mathieu\AppData\Local\Temp\pytest-of-Mathieu\pytest-134\test_export_evokeds_to_mff_Fal1\evoked.mff' exists already

How do you feel about the idea of deleting the file prior to calling mffpy.Writer? I could also open a PR for a feature request on mffpy.

[larsoner]

How do you feel about the idea of deleting the file prior to calling mffpy.Writer? I could also open a PR for a feature request on mffpy.

I would open an issue upstream and for now delete at our end until they release a version that supports overwriite (if ever) and at that point triage what to do in MNE based on the version of mmfpy installed

[mscheltienne]

@larsoner Alright, so let's leave this PR as it is. It doesn't yet include any change to the evoked export.

I will open one upstream with the feature request + a second one on MNE with the change to include to export of evoked objects when the upstream change is merged and released.

[larsoner]

@larsoner Alright, so let's leave this PR as it is. It doesn't yet include any change to the evoked export.

Sorry, I meant for now delete the file at our end (not the code for overwrite). In other words, do add overwrite to export_evokeds and export_evokeds_mff in this PR

[mscheltienne]

Oh alright, I will add that 'hack' a bit later this evening as I open the PR upstream.

[mscheltienne]

Solved, I am not familiar with mffpy, and fname is in fact.. a directory.

[larsoner]

FYI if you go to the "Files changed" pane, you can "add suggestion to batch" to commit all suggestions at once rather than going one-by-one (you can't do it from the main PR page)

[mscheltienne]

@larsoner Yes I just noticed by hovering my mouse on the add to batch button midway through ahah Next time ;)

[mscheltienne]

I'm adding one more quick fix to when pathlib.Path instances are passed as fname. eeglabio only supports str. For now, when a Path is provided, a TypeError is raised by scipy.

    raw.export(output_fname, fmt='eeglab')

  File "<decorator-gen-207>", line 24, in export

  File "C:\Users\Mathieu\Documents\pyvenv\neurotin\lib\site-packages\mne\io\base.py", line 1501, in export
    export_raw(fname, self, fmt, physical_range=physical_range,

  File "<decorator-gen-575>", line 24, in export_raw

  File "C:\Users\Mathieu\Documents\pyvenv\neurotin\lib\site-packages\mne\export\_export.py", line 46, in export_raw
    _export_raw(fname, raw)

  File "C:\Users\Mathieu\Documents\pyvenv\neurotin\lib\site-packages\mne\export\_eeglab.py", line 29, in _export_raw
    eeglabio.raw.export_set(

  File "C:\Users\Mathieu\Documents\pyvenv\neurotin\lib\site-packages\eeglabio\raw.py", line 78, in export_set
    savemat(fname, eeg_d, appendmat=False)

  File "C:\Users\Mathieu\Documents\pyvenv\neurotin\lib\site-packages\scipy\io\matlab\mio.py", line 298, in savemat
    MW.put_variables(mdict)

  File "C:\Users\Mathieu\Documents\pyvenv\neurotin\lib\site-packages\scipy\io\matlab\mio5.py", line 892, in put_variables
    self._matrix_writer.write_top(var, name.encode('latin1'), is_global)

  File "C:\Users\Mathieu\Documents\pyvenv\neurotin\lib\site-packages\scipy\io\matlab\mio5.py", line 633, in write_top
    self.write(arr)

  File "C:\Users\Mathieu\Documents\pyvenv\neurotin\lib\site-packages\scipy\io\matlab\mio5.py", line 653, in write
    raise TypeError('Could not convert %s (type %s) to array'

TypeError: Could not convert C:\Users\Mathieu\Documents\datasets\neurotin\test-raw.set (type <class 'pathlib.WindowsPath'>) to array

[larsoner]

I'm adding one more quick fix to when pathlib.Path instances are passed as fname. eeglabio only supports str. For now, when a Path is provided, a TypeError is raised by scipy.

To me this suggests _check_fname is not used properly somewhere, its output type is always str

mne-python/mne/utils/check.py

Lines 149 to 157 in a8522d2

	def _check_fname(fname, overwrite=False, must_exist=False, name='File',
	need_dir=False):
	"""Check for file existence, and return string of its absolute path."""
	_validate_type(fname, 'path-like', name)
	fname = str(
	Path(fname)
	.expanduser()
	.absolute()
	)

[mscheltienne]

My bad.. wrong venv.. _check_fname is properly used everywhere now I think, so this problem when a Path is provided should not occur anymore. I'm leaving the 2 additional test cases in any way.

[mscheltienne]

@larsoner This one should be good to go.

[larsoner]

Thanks @mscheltienne !