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

[WIP] : simplify fpath #721

Draft
wants to merge 17 commits into
base: main
Choose a base branch
from
Draft

Conversation

agramfort
Copy link
Member

attempt to close #712

cc @adam2392 @dmalt

@adam2392 looking more closely at this issue I am not sure what
is the intended difference between .basename and .fpath attribute
in BIDSPath.

CI will be red here but it starts the ball rolling.

@adam2392 you can push to this PR if you want

mne_bids/path.py Outdated Show resolved Hide resolved
mne_bids/path.py Outdated
@@ -409,6 +410,8 @@ def fpath(self):
# get the inner-most BIDS directory for this file path
data_path = self.directory

assert self.root is not None
Copy link
Member

Choose a reason for hiding this comment

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

We could make use of MNE's _validate_type()

@adam2392
Copy link
Member

adam2392 commented Mar 7, 2021

@adam2392 looking more closely at this issue I am not sure what
is the intended difference between .basename and .fpath attribute
in BIDSPath.

IIRC basename is like os.path.basename, whereas fpath is like a pathlib.Path essentially. Furthermore looking at the code...

  • basename only gets you the filename (you don't know datatype)
  • basename gets you a string object
  • fpath gets you a Path object and either the full, or relative path

@adam2392 you can push to this PR if you want

I will try to set aside time soon.

@agramfort
Copy link
Member Author

@adam2392 should basename == BIDSPath.fpath.name ?

@adam2392
Copy link
Member

@adam2392 should basename == BIDSPath.fpath.name ?

I believe so

@adam2392
Copy link
Member

@agramfort I don't believe I have write access here?

@agramfort
Copy link
Member Author

if you have commit right on mne-bids you do

@hoechenberger
Copy link
Member

@agramfort I don't believe I have write access here?

I've just granted you write access to this repo, can you push here now?

@hoechenberger hoechenberger added this to the 0.8 milestone Mar 12, 2021
@adam2392
Copy link
Member

Stupid question: how do I fetch this PR?

(mne) adam2392@Adams-MacBook-Pro mne-bids % git remote  -v
agram   https://github.com/agramfort/mne-bids.git (fetch)
agram   https://github.com/agramfort/mne-bids.git (push)
dev     https://github.com/adam2392/mne-bids.git (fetch)
dev     https://github.com/adam2392/mne-bids.git (push)
upstream        https://github.com/mne-tools/mne-bids.git (fetch)
upstream        https://github.com/mne-tools/mne-bids.git (push)
(mne) adam2392@Adams-MacBook-Pro mne-bids % git fetch upstream pull/721/head:simplify_fpath
fatal: Refusing to fetch into current branch refs/heads/simplify_fpath of non-bare repository
(mne) adam2392@Adams-MacBook-Pro mne-bids % git fetch upstream pull/721/agramfort:simplify_fpath
fatal: couldn't find remote ref pull/721/agramfort

@sappelhoff
Copy link
Member

I recommend that you use https://github.com/cli/cli ... it simplifies these sorts of things remarkably well. See the following screenshot (upper right corner in the image):

image

but for the record, with just git you would do:

git fetch agram simplify_fpath
git checkout simplify_fpath
<do your changes>
git push -u origin my_simplify_fpath
<do a PR to alex's GitHub fork>

If you have maintainer rights (I think you don't right now) you can also push to alex's fork directly ... pretty much what I did for your fork in #717

@hoechenberger
Copy link
Member

Stupid question: how do I fetch this PR?

git fetch --all
git switch simplify_fpath

@hoechenberger
Copy link
Member

If you have maintainer rights (I think you don't right now) you can also push to alex's fork directly

I've granted @adam2392 write rights, I thought this would suffice … well, let's see!

@adam2392
Copy link
Member

My understanding of our simplification is to entirely remove the attempt at "matching basenames" inside fpath, and simply have fpath be an appendage of BIDSPath.directory and BIDSPath.basename. Is this correct?

@agramfort
Copy link
Member Author

agramfort commented Mar 15, 2021 via email

@adam2392 adam2392 marked this pull request as ready for review March 17, 2021 16:52
@adam2392
Copy link
Member

I am running into an error with the emptyroom stuff, which I am now out of my depth as to how to address the issue. Those functions use fpath to infer some dataset:

FAILED mne_bids/tests/test_path.py::test_find_empty_room - TypeError: expected str, bytes or os.PathLike object, not NoneType
FAILED mne_bids/tests/test_path.py::test_find_emptyroom_ties - TypeError: expected str, bytes or os.PathLike object, not NoneType
FAILED mne_bids/tests/test_path.py::test_find_emptyroom_no_meas_date - TypeError: expected str, bytes or os.PathLike object, not NoneType

@hoechenberger would you be able to assist?

mne_bids/tests/test_path.py Outdated Show resolved Hide resolved
mne_bids/tests/test_path.py Outdated Show resolved Hide resolved
@adam2392 adam2392 changed the title WIP : simplify fpath [MRG] : simplify fpath Mar 21, 2021
@adam2392 adam2392 changed the title [MRG] : simplify fpath [WIP] : simplify fpath Mar 21, 2021
@agramfort
Copy link
Member Author

@adam2392 you need to rebase.

@hoechenberger
Copy link
Member

I've resolved the merge conflict. Looking into the emptyroom stuff now.

Base automatically changed from master to main March 26, 2021 13:40
@sappelhoff sappelhoff marked this pull request as draft July 8, 2021 13:40
@sappelhoff sappelhoff modified the milestones: 0.8, 0.9 Jul 8, 2021
@hoechenberger hoechenberger modified the milestones: 0.9, 0.10 Nov 10, 2021
@hoechenberger
Copy link
Member

Moving to 0.10 milestone

@sappelhoff sappelhoff modified the milestones: 0.11, 0.12 Oct 8, 2022
@sappelhoff sappelhoff modified the milestones: 0.12, 0.13 Dec 18, 2022
@sappelhoff sappelhoff modified the milestones: 0.13, future (or never) Jul 27, 2023
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.

Splits are fetched by BIDSPath.fpath only when extension (and/or) suffix are missing
4 participants