mne-python
mne-python copied to clipboard
mne-tools
Unify treatment of pathlike objects
Describe the bug
Across mne pathlike objects are currently treated inconsistently. In many place, paths are tested for and accepted being str or pathlib.Path. In other places, only str is accepted and the functions fails for Path objects. The problem occured in my pipelines in multiple places and seemingly random, where different function from the same context behaved differently.
Steps to reproduce
One example where a Path object fails is mne.make_forward_solution(info, ...) where info can either be an instance of Info or a string representing a path, but not a pathlib.path. In particular, the in mne.forward._make_forward.py:
564 if not isinstance(info, (Info, str)):
565 raise TypeError('info should be an instance of Info or string')
Expected results
Judging from the mne code the intention appears to be that both str and Path representations of a path are accepted everywhere.
Additional information
I would like to get your feedback, whether there are reasons why this is not treated consistently, but I gather that this is oversight due to the shear amount of places to check. I also offer to try and hunt down places that need changing and submit a pull request. However, there seem to be variations to how, when and where paths are checked in the code. It might make sense to unify that as well. For example, I this reference in mne.io.write.start_file:
308 fname = _fn35(fname)
That points to this code in mne.fixes:
def _fn35(fname):
try:
from py._path.common import PathBase
except ImportError:
pass
else:
if isinstance(fname, PathBase):
fname = str(fname)
if isinstance(fname, Path):
fname = str(fname)
return fname
To be honest, I do not understand the purpose of this particular function, because all it does is call str(fname) and even if it doesn't it most certainly would not hurt to do it (a str of a str is still a str). So the function could probably just be removed altogether and replaced with str(fname).
Your thoughts?
Across mne pathlike objects are currently treated inconsistently. In many place, paths are tested for and accepted being str or pathlib.Path. In other places, only str is accepted and the functions fails for Path objects. The problem occured in my pipelines in multiple places and seemingly random, where different function from the same context behaved differently.
We have slowly been converting all of these to:
_validate_type(info, (Info, 'path-like'), 'info')
if not isinstance(info, Info): # path-like
info = read_info(info)
and under the hood, read_info should do at some point something like:
info = _check_fname(info, must_exist=True, overwrite='read', name='info')
But we haven't made this transition everywhere, as you've seen with our forward code.
For example, I this reference in mne.io.write.start_file [about _fn35]
This looks like old Python3.5 cruft to me that should make use of 'path-like' / _check_fname
Would you be up for trying a PR for these fixes?
I can try, I am unsure, however, how they should exactly be fixed. Is this code snippet that you mentioned the go-to solution for all pathlike checks?
_validate_type(info, (Info, 'path-like'), 'info')
if not isinstance(info, Info): # path-like
info = read_info(info)
About the _fn35 function: so that should essentially be replaced with _check_fname, did I understand that correctly?
Is this code snippet that you mentioned the go-to solution for all pathlike checks?
Most reading checks, yes. If you look in our code (e.g., git grep _check_fname) you can find many existing good examples of how to handle paths
About the _fn35 function: so that should essentially be replaced with _check_fname, did I understand that correctly?
Yeah, I think that should work!
I don't understand the Info type here – what does that have to do with str and pathlib.Path types?
Presumably in this part of the code you can either pass an Info obj directly, or a path to a file with Info that can be read
@dafrose are you still hoping to work on this? If not, we might assign someone else to do it at a sprint at the end of June.
I'm happy to work on this one:
- mne.fixes._fn35 was removed in this PR: https://github.com/mne-tools/mne-python/pull/7638.
- I'll look into hunting down where _check_fname should be added.
Note that it was already a bit improved in https://github.com/mne-tools/mne-python/pull/11473
