mne-tools / mne-tools/mne-python
Unify treatment of pathlike objects
Nobody has claimed this yet.
- Dominant language
- Python
- Stars
- 3.5k
- Forks
- 1.6k
- Avg merge
- 1d 6h
- Merged PRs (30d)
- 100
Description
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?
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with the type check in mne/forward/_make_forward.py and inspect mne.io.write.start_file plus mne.fixes._fn35. Trace other path checks across the affected APIs before deciding on a consistent scope. Done means the agreed pathlike behavior is applied consistently and the affected cases are verified.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- python
- Domain
- api, backend
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Stale
- Clarity
- Needs clarification
- Newbie friendliness
- 25/100