mne-tools / mne-tools/mne-python

Unify treatment of pathlike objects

Open
#10,496 9 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

BUG sprint-2023
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

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.