MRG: Expand ~ in _check_fname() by hoechenberger · Pull Request #9613 · mne-tools/mne-python

hoechenberger · 2021-07-26T16:27:26Z

Currently just a proof of concept for FIFF reading. Will expand to other places across the entire code base. Feel super free to help me and push to this branch.

Currently just a proof of concept for FIFF reading.

larsoner · 2021-07-26T16:29:12Z

Sounds like a good idea to me! Then once we standardize _check_fname use in all functions that read and write (which we can do progressively / not in this PR) then ~ should work everywhere, which is nice!

cbrnr · 2021-07-26T16:31:28Z

Can we use pathlib.resolve() instead?

hoechenberger · 2021-07-26T16:31:47Z

Then once we standardize _check_fname use in all functions that read and write (which we can do progressively / not in this PR)

Oh, my intention actually was to get this to work in this PR :) But maybe that's too ambitious. Should I limit things to just FIFF for now (as currently implemented) in this PR?

larsoner · 2021-07-26T16:33:00Z

I think it's okay to go for as much as you're up for doing in this PR

Can we use pathlib.resolve() instead?

No idea what the implications are of switching from our os.path-based functions to pathlib.resolve

hoechenberger · 2021-07-26T16:33:49Z

Can we use pathlib.resolve() instead?

That's resolving symlinks, not expanding ~.

If the intention of the question is, "can we start moving our path handling over to pathlib", I'd be +9999, it's so much nicer and cleaner than dealing with strings and os.path in many situations.

hoechenberger · 2021-07-26T16:35:35Z

Can we use pathlib.resolve() instead?

That's resolving symlinks, not expanding ~.

Maybe you were thinking of Path.expanduser()?

cbrnr · 2021-07-26T17:14:18Z

Maybe you were thinking of Path.expanduser()?

Yes!

hoechenberger · 2021-07-26T17:30:21Z

Maybe you were thinking of Path.expanduser()?

Yes!

I'm using that one in the test, it's something 😅🙈

…xpanduser

hoechenberger · 2021-07-28T17:03:28Z

Ok I think I'm now covering a large number (all? probably forgot some!) data types we deal with in MNE.

I'm too lazy to add tests for all of them now :( Somebody willing to give me a hand?

hoechenberger · 2021-07-28T17:10:11Z

Also sometimes it's not absolutely clear at which function nesting level to expand the fname. I'm trying to do it early, but I'm not always super consistent here.

mne/cov.py

mne/epochs.py

mne/evoked.py

mne/io/eeglab/eeglab.py

mne/io/fiff/tests/test_raw_fiff.py

larsoner · 2021-07-29T17:01:39Z

mne/minimum_norm/inverse.py

    check_fname(fname, 'inverse operator', ('-inv.fif', '-inv.fif.gz',
                                            '_inv.fif', '_inv.fif.gz'))


Might be better / simpler just to make check_fname call _check_fname internally and return the str-ified absolute path, then our code ends up more DRY. It seems like every time we check_fname we probably want to call _check_fname anyway...

Or we could go the other way and make _check_fname accept a ext argument, and have it call check_fname internally. I think I'd rather go that way.

Yes I was thinking the same, however I don't want to touch this in this PR to avoid unwelcome surprises and to keep the diff smaller.

Can you open an issue about this? It would make a hopefully straightforward follow-up PR

Issue filed under #9819

mne/source_estimate.py

mne/utils/docs.py

…xpanduser

hoechenberger · 2021-10-06T12:45:25Z

Ok this is ready to go from my end!

hoechenberger · 2021-10-06T14:04:02Z

@agramfort I also tested with your Forward solution notebook, passing Path objects around like you suggested. All working nicely!

agramfort

@larsoner merge if happy

thx @hoechenberger 👏

larsoner

Other than some minor stuff, LGTM!

larsoner · 2021-10-06T15:10:10Z

mne/epochs.py

-        if isinstance(fname, str):
-            check_fname(fname, 'epochs', ('-epo.fif', '-epo.fif.gz',
-                                          '_epo.fif', '_epo.fif.gz'))
+        if _check_path_like(fname):


Usually _check_* functions raise errors, this would be better as _path_like(...) I think

Not the case for this function. Checked git blame and … well, it was me who added it this way 2 years ago. Pushing a rename.

Renamed to _path_like. It's still residing check.py, which should probably be reconsidered at some time in the future.

larsoner · 2021-10-06T15:12:41Z

mne/minimum_norm/inverse.py

    check_fname(fname, 'inverse operator', ('-inv.fif', '-inv.fif.gz',
                                            '_inv.fif', '_inv.fif.gz'))


Can you open an issue about this? It would make a hopefully straightforward follow-up PR

larsoner · 2021-10-06T15:13:57Z

mne/source_estimate.py

        """
        _validate_type(fname, 'path-like', 'fname')
-        fname = str(fname)
+        fname = _check_fname(fname=fname, overwrite=True)


Any place you have had to add an overwrite=True like this can you add a # TODO comment saying we should add an overwrite kwarg to the function?

larsoner · 2021-10-06T15:15:30Z

mne/utils/check.py

                 need_dir=False):
    """Check for file existence, and return string of its absolute path."""
    _validate_type(fname, 'path-like', name)
+    fname = op.expanduser(fname)


now the modification of fname happens it two places -- here and below during the return. Can you move the fname = str(op.abspath(fname)) after this line, then just return fname at the end? That way it's immediately clear what all operations we are actually doing to fname before returning it.

I've reworked the logic as you suggested, and I'm operating on a Path now before casting to a string, as I felt that the method chaining available via Path made the sequence of operations easier to read.

mne/utils/tests/test_config.py

…xpanduser

hoechenberger · 2021-10-08T12:01:18Z

@larsoner This should be good to go.

agramfort · 2021-10-08T13:38:26Z

thx @hoechenberger !

* upstream/main: MAINT: Update broken link, fix rendering (mne-tools#9829) Ensure plot_ica_sources() always plots traces of rejected ICs on top (mne-tools#9823) Improve plot_ica_sources() docstring (mne-tools#9825) MRG: Fix docstring for plot_ica_components() (mne-tools#9826) unpin jsonschema and filter its warning instead (mne-tools#9822) Add warning for SNIRF files with != 2 wavelengths (mne-tools#9817) add show_scalebars param to epochs.plot() (mne-tools#9815) MRG: Allow _plot_mri_contours() to return arrays (mne-tools#9818) MRG: Expand ~ in _check_fname() (mne-tools#9613) Improve ICA.plot_overlay() docstrings (mne-tools#9820) WIP, MAINT: Fix CircleCI (again) (mne-tools#9814) MRG, ENH: Add options to fit_dipole (mne-tools#9810) Rework Reports (new history) (mne-tools#9754) MRG, CI: Use VTK pre (mne-tools#9803)

Followup of mne-tools#9613

* Expand ~ in Report file operations Followup of #9613 * Update changelog * Fix report opening

WIP: Expand ~ in _check_fname()

6cb7bea

Currently just a proof of concept for FIFF reading.

hoechenberger added 3 commits July 28, 2021 18:28

docstyle

5006a4a

Add more data types

996f16b

Merge branch 'main' of https://github.com/mne-tools/mne-python into e…

438ee16

…xpanduser

hoechenberger added 2 commits July 28, 2021 19:03

Fix typo

2c0d14c

Add another one

b4d589f

hoechenberger added 6 commits July 28, 2021 19:53

Fixes

98f495d

Fix fix fix 🐞

540cc57

Fix FIFF

4567d42

Fix KIT

97cbbe1

Fix STC

5669af2

Fix test_plot_evoked_cov()

80bd7fb

larsoner reviewed Jul 29, 2021

View reviewed changes

larsoner added this to the 0.24 milestone Sep 1, 2021

hoechenberger added 6 commits September 29, 2021 18:39

Merge branch 'main' of https://github.com/mne-tools/mne-python into e…

704c9f1

…xpanduser

Merge branch 'main' of https://github.com/mne-tools/mne-python into e…

b1d4df3

…xpanduser

Fix EEGLAB

44b3863

Don't touch users' home directory during tests

70dd640

Fix docstring

1356a2d

Fix type check

1c49564

hoechenberger added 5 commits October 5, 2021 22:24

Fix for subjects_dir=None

e389a27

Fix tests

2c989c5

Try to fix tests on Windows

2d56533

Try to fix another test on Windows

6e874f5

Update changelog [skip azp][skip azure]

16e865a

agramfort approved these changes Oct 6, 2021

View reviewed changes

larsoner requested changes Oct 6, 2021

View reviewed changes

hoechenberger mentioned this pull request Oct 8, 2021

Modify check_fname() to internally call _check_fname(), or vice versa #9819

Open

hoechenberger added 10 commits October 8, 2021 11:26

Rename _check_path_like() -> _path_like()

921d1e1

Add TODO comments to remind us of missing overwrite params

8ddd827

Rephrase changelog entry

b22313d

Slightly chang _check_fname() logic as suggested in review

25a85a4

Merge branch 'main' of https://github.com/mne-tools/mne-python into e…

a80604e

…xpanduser

It seems something went wrong when merging main 🤕

bd76c1e

Fix a typo

8a0c8b0

Fix more issues resulting from merging main

48c2504

And another one

41555e2

Hopefully the last one

622da4d

larsoner approved these changes Oct 8, 2021

View reviewed changes

agramfort merged commit 96e830b into mne-tools:main Oct 8, 2021

hoechenberger deleted the expanduser branch October 8, 2021 13:57

hoechenberger added a commit to hoechenberger/mne-python that referenced this pull request Oct 14, 2021

Expand ~ in Report file operations

2a8554b

Followup of mne-tools#9613

hoechenberger mentioned this pull request Oct 14, 2021

MRG: Expand ~ in Report file operations #9845

Merged

larsoner pushed a commit that referenced this pull request Oct 14, 2021

MRG: Expand ~ in Report file operations (#9845)

022f1a8

* Expand ~ in Report file operations Followup of #9613 * Update changelog * Fix report opening

sappelhoff mentioned this pull request Dec 6, 2021

Raw.save logs too many "Overwriting existing file." infos #10094

Closed

		check_fname(fname, 'inverse operator', ('-inv.fif', '-inv.fif.gz',
		'_inv.fif', '_inv.fif.gz'))

Uh oh!

Conversation

hoechenberger commented Jul 26, 2021

Uh oh!

larsoner commented Jul 26, 2021

Uh oh!

cbrnr commented Jul 26, 2021

Uh oh!

hoechenberger commented Jul 26, 2021

Uh oh!

larsoner commented Jul 26, 2021

Uh oh!

hoechenberger commented Jul 26, 2021

Uh oh!

hoechenberger commented Jul 26, 2021

Uh oh!

cbrnr commented Jul 26, 2021

Uh oh!

hoechenberger commented Jul 26, 2021

Uh oh!

hoechenberger commented Jul 28, 2021

Uh oh!

hoechenberger commented Jul 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hoechenberger commented Oct 6, 2021

Uh oh!

hoechenberger commented Oct 6, 2021

Uh oh!

agramfort left a comment

Choose a reason for hiding this comment

Uh oh!

larsoner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hoechenberger Oct 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hoechenberger commented Oct 8, 2021

Uh oh!

agramfort commented Oct 8, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

hoechenberger commented Jul 28, 2021 •

edited

Loading

hoechenberger Oct 8, 2021 •

edited

Loading