-
Notifications
You must be signed in to change notification settings - Fork 92
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 Rework mne bids path match #1355
base: main
Are you sure you want to change the base?
Conversation
…h are not files. Are there more folders in different datatypes allowed?
Hello! 👋 Thanks for opening your first pull request here! |
pyproject.toml
Outdated
@@ -52,6 +52,7 @@ doc = [ | |||
"numpydoc", | |||
"openneuro-py", | |||
"pandas", | |||
"pathlib", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's part of the standard library and doesn't need to be listed here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thx, this comes up though 🤔
/home/circleci/project/mne_bids/path.py:docstring of mne_bids.BIDSPath.root:1: WARNING: py:class reference target not found: pathlib._local.Path [ref.class]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #1358 (comment) . #1353 includes a work around for this but hasn't been merged yet
412aaef
to
09ba143
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1355 +/- ##
==========================================
- Coverage 97.40% 97.37% -0.04%
==========================================
Files 40 40
Lines 8915 8924 +9
==========================================
+ Hits 8684 8690 +6
- Misses 231 234 +3 ☔ View full report in Codecov by Sentry. |
thanks @waldie11! we have a bit of a backlog right now so please don't worry if we take a bit longer to get back to you. |
@sappelhoff happy new year! |
4863705
to
c71a41f
Compare
for more information, see https://pre-commit.ci
PR Description
The workflow of using
pathlib.Path.rglob
and subsequently filtering i.e. by each path starting withroot/sub-*
is rather non-performant, especially on polluted storage trees.For starters, by using
glob.glob
recursively directly, it is possible to apply this filter equivalent immediately.In an extension, I am suggesting to introduce the option of
include_match
toget_entity_vals
.Open to suggestions, or stripping this down.
Remark: I failed to join the CI as my email adress is considered work mail for some reason by MS.
Merge checklist
Maintainer, please confirm the following before merging.
If applicable: