Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use pathlib.Path instead of os.path to handle files and folders #11473

Merged
merged 145 commits into from
Feb 21, 2023

Conversation

mscheltienne
Copy link
Member

@mscheltienne mscheltienne commented Feb 14, 2023

The MNE codebase uses a mix of strings (with the help of os.path) and pathlib.Path objects to handle files and folders.
I believe it would be beneficial to move to pathlib.Path objects entirely.

Summary of the PR here.


Current list of tests and associated functions incompatible with Path objects:

test_transforms: 
	::test_fs_xfm
	387: xfm, kind = _read_fs_xfm(str(fname))
	393: xfm_read, kind_read = _read_fs_xfm(str(fname_out))
	399: xfm_read, kind_read = _read_fs_xfm(str(fname_out))
	406: _read_fs_xfm(str(fname_out))
	409: _read_fs_xfm(str(fname_out))

viz/_brain/test_brain
	::test_brain_init
	319: brain.add_label(str(fname_label))
	435: brain.add_annotation(str(annots[-1]))
	442: brain.add_annotation(str(a), b, p, color=color)

report/test_report
	::test_open_report
	Not sure what is failing when hdf5, .fname and .subjects_dir are Path. 
	Probably requires those 2 last attributes to load Path from disk.

	::test_scraper
	719: block_vars = dict(image_path_iterator=(img for img in [str(img_fname)]),
                               example_globals=dict(a=1), target_file=target_file)
               -> impacts L723: rshe 'destination' argument of maxwell_filtert = scraper(block, block_vars, gallery_conf)

viz/_brain/test_notebook
	::test_notebook_interactive
        88: brain._renderer.actions['movie_field'].value = str(movie_path)
        89: brain._renderer.actions['screenshot_field'].value = str(screenshot_path)

@mscheltienne mscheltienne marked this pull request as draft February 14, 2023 13:21
@drammock
Copy link
Member

Yes please do! It's been planned to switch entirely to pathlib but nobody had the time so far

@mscheltienne
Copy link
Member Author

Great, the first step is perfect for when I'm out of energy and don't want to think too hard ;)
I'll continue this PR little by little.

str_data = data.encode('latin1')
str_data = str(data).encode('latin1')
Copy link
Member Author

@mscheltienne mscheltienne Feb 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note about this change:

When writing a src, the field "mri_volume_name" could be a Path which can not be encoded to "latin1".
IMO, it is safer to cast to string before writing than to chase every case where something else than a string might be provided to write_string.

If reverted, a test in tests/test_source_space will break, so we can always fix this issue differently if we want to.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed that casting to str before write operations is the correct fix. I think I've hit this problem before (and obviously didn't fix it everywhere 😉).

@larsoner
Copy link
Member

The MPL deprecations should be fixed/worked around by #11497, but this one does seem related:

/home/circleci/project/tutorials/inverse/80_brainstorm_phantom_elekta.py failed leaving traceback:
Traceback (most recent call last):
  File "/home/circleci/project/tutorials/inverse/80_brainstorm_phantom_elekta.py", line 94, in <module>
    mne.viz.plot_alignment(
  File "<decorator-gen-122>", line 12, in plot_alignment
  File "/home/circleci/project/mne/viz/_3d.py", line 780, in plot_alignment
    _plot_mri_fiducials(renderer, mri_fiducials, subjects_dir, subject,
  File "/home/circleci/project/mne/viz/_3d.py", line 1033, in _plot_mri_fiducials
    fid_loc = _fiducial_coords(mri_fiducials, FIFF.FIFFV_COORD_MRI)
  File "/home/circleci/project/mne/viz/_3d.py", line 65, in _fiducial_coords
    points = [p for p in points if p['coord_frame'] == coord_frame]
TypeError: 'PosixPath' object is not iterable

probably something somewhere does not look for path-like but instead str for a filename...

@mscheltienne
Copy link
Member Author

mscheltienne commented Feb 21, 2023

Looks like it's all green, modulo #11497. A quick summary of the changes:

Occurrences of os.path in mne-python/mne:

  • on main: 364 matches in 213 files
  • on this PR: 187 matches in 88 files

Changes:

  • Use pathlib instead of os.path in the tests files. The exceptions are:
    • mne/io/tests/test_constants.py
    • mne/io/eeglab/tests/test_eeglab.py
    • mne/utils/tests/test_testing.py
    • mne/datasets/sleep_physionet/tests/test_physionet.py
    • mne/viz/tests/test_scraper.py
  • Cast to string before writing a string FIFF tag. c.f. this comment.
  • Fixes incompatibilities with Path in:
    • destination argument of maxwell_filter
    • I/O for layouts, add argument overwrite=False for Layout.save()
    • head_pos argument of simulate_raw
    • EGI evoked reader read_evokeds_mff
    • src argument of plot_bem
    • mri_fiducials argument of plot_alignment
    • hsp, elp and mrk argument of the KIT reader
    • events argument of the EpochsEEGLAB reader
  • Change the behavior of _check_fname, _find_trans and get_subjects_dir that now return a Path instead of a string.

Especially for _check_fname and get_subjects_dir, I either:

  • in small functions, refactored/adapted the code to work with Path
  • in large functions where the variable was extensively used OR in long chain of functions OR when I was not confident about the change, I converted the value back to string retaining the legacy behavior.

Examples can be found by searching for str(_check_fname( e.g. https://github.com/mscheltienne/mne-python/blob/b1facb9076c9afb3eee3f52f8414eef801f649ad/mne/source_estimate.py#L250

And that should be a good stopping point for this PR 😄

@mscheltienne mscheltienne marked this pull request as ready for review February 21, 2023 16:24
@larsoner
Copy link
Member

I took a cursory look at the code and it looks reasonable. Your choices also sound good to me. CIs were green before I merged main into your branch. I'll merge now with [circle deploy] and we can make sure CircleCI agrees things are all good here. Thanks for this big effort and progress @mscheltienne !

@larsoner larsoner merged commit 5a97c2f into mne-tools:main Feb 21, 2023
@hoechenberger
Copy link
Member

Super nice. Thanks @mscheltienne

@mscheltienne mscheltienne deleted the os_path branch February 21, 2023 21:36
@agramfort
Copy link
Member

whoooot 200 files changes 🚀

@hoechenberger
Copy link
Member

I believe we forgot a changelog entry – @mscheltienne could you add one?

@mscheltienne
Copy link
Member Author

@hoechenberger Done in #11499, but I made it very general and did not include cross-ref to the different functions now compatible with Paths.

larsoner added a commit to drammock/mne-python that referenced this pull request Feb 22, 2023
* upstream/main: (46 commits)
  Fix docstrings by replacing str with path-like and fix double backticks for formatting (mne-tools#11499)
  Use pathlib.Path instead of os.path to handle files and folders [circle deploy] (mne-tools#11473)
  MAINT: Fix Circle [circle deploy] (mne-tools#11497)
  MAINT: Use mamba in CIs (mne-tools#11471)
  Updating documentation to clarify full vs half-bandwidth and defaults in time_frequency.multitaper.py (mne-tools#11479)
  Fix typo in tutorial (mne-tools#11498)
  Typo fix and added colons before code (mne-tools#11496)
  [MRG] ENH/DOC: demo custom spectrum creation (mne-tools#11493)
  Accept only left-clicks for adding annotations (mne-tools#11491)
  [BUG, MRG] Fix pial surface loading, logging in locate_ieeg (mne-tools#11489)
  [ENH] Added unit_role to add non-breaking space between magnitude  and units (mne-tools#11469)
  MAINT: Fix CircleCI build (mne-tools#11488)
  [DOC] Updated decoding.SSD documentation and internal variable naming (mne-tools#11475)
  Typo fix (mne-tools#11485)
  [MRG] Forward argument axes from plot_sensors to DigMontage.plot (mne-tools#11470)
  [MRG] Improve error message raised on channels missing from DigMontage (mne-tools#11472)
  MAINT: Deal with pkg_resources usage bugs (mne-tools#11478)
  Add object array support and docstring (mne-tools#11465)
  [ENH] Adjusted SSD algorithm to support non-full rank data (mne-tools#11458)
  [BUG] fix nibabel reference (mne-tools#11467)
  ...
larsoner added a commit to larsoner/mne-python that referenced this pull request Mar 1, 2023
* upstream/main: (264 commits)
  BUG: Fix deprecated API usage in example (mne-tools#11512)
  Deprecate 'kind' and 'path' in favor of 'fname' in the layout reader (mne-tools#11500)
  EGI/MFF events outside EEG recording should not break the code (mne-tools#11505)
  fixed annotations error on export (mne-tools#11435)
  DOC: Update installer links [skip azp] [skip actions] [skip cirrus] (mne-tools#11506)
  BUG: updates for MPL 3.7 compatibility (mne-tools#11409)
  Fix docstrings by replacing str with path-like and fix double backticks for formatting (mne-tools#11499)
  Use pathlib.Path instead of os.path to handle files and folders [circle deploy] (mne-tools#11473)
  MAINT: Fix Circle [circle deploy] (mne-tools#11497)
  MAINT: Use mamba in CIs (mne-tools#11471)
  Updating documentation to clarify full vs half-bandwidth and defaults in time_frequency.multitaper.py (mne-tools#11479)
  Fix typo in tutorial (mne-tools#11498)
  Typo fix and added colons before code (mne-tools#11496)
  [MRG] ENH/DOC: demo custom spectrum creation (mne-tools#11493)
  Accept only left-clicks for adding annotations (mne-tools#11491)
  [BUG, MRG] Fix pial surface loading, logging in locate_ieeg (mne-tools#11489)
  [ENH] Added unit_role to add non-breaking space between magnitude  and units (mne-tools#11469)
  MAINT: Fix CircleCI build (mne-tools#11488)
  [DOC] Updated decoding.SSD documentation and internal variable naming (mne-tools#11475)
  Typo fix (mne-tools#11485)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants