Revert "WIP Rework mne bids path match"

[drammock]

Reverts #1355

This changed behavior in a way that broke downstream tests, see mne-tools/mne-bids-pipeline#1059

cc @waldie11 @sappelhoff

[codecov]

Codecov Report

Attention: Patch coverage is 83.33333% with 2 lines in your changes missing coverage. Please review.

Project coverage is 97.43%. Comparing base (b75e4ca) to head (517707a).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
mne_bids/path.py	83.33%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1376      +/-   ##
==========================================
+ Coverage   97.37%   97.43%   +0.05%     
==========================================
  Files          40       40              
  Lines        8966     8966              
==========================================
+ Hits         8731     8736       +5     
+ Misses        235      230       -5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[sappelhoff]

Interesting, thanks for the report @drammock. Is it possible that this is an issue in the mne-bids-pipeline rather than mne-bids?

@waldie11 can you have a look as well please?

Finally (also @hoechenberger), should we perhaps run a part of the mne-bids-pipeline tests as part of the mne-bids testsuite?

[larsoner]

Finally (also @hoechenberger), should we perhaps run a part of the mne-bids-pipeline tests as part of the mne-bids testsuite?

The mne-bids-pipeline tests are generally kind of long and require a lot of data transfer, so we run most of them on CircleCI (though there are a couple of datasets that run on GHA). My sense is that so far the incidence of breakage has been fairly low so we could hold off and wait to see if we see more frequent breakages over there due to MNE-BIDS changes. But it looks like it would only be ~30 lines to add an equivalent test here -- wouldn't need to test the "caching" / rerun and could, so would probably just be a simplified (e.g., one OS, one dataset) version of:

https://github.com/mne-tools/mne-bids-pipeline/blob/58f4b76de169c02a824c8462c60406ce1d2f46b7/.github/workflows/tests.yml#L31-L64

I could make a quick PR if it would help and we could see if the runtime is acceptable etc. It wouldn't catch all breakages but probably would catch some.

[larsoner]

Maybe a cause of mne-tools/mne-nirs#601 failures as well?

[sappelhoff]

I could make a quick PR if it would help and we could see if the runtime is acceptable etc. It wouldn't catch all breakages but probably would catch some.

that would be much appreciated!

Maybe a cause of mne-tools/mne-nirs#601 failures as well?

😳 seems like there is a lot of stuff that our mne-bids tests do not catch.

[larsoner]

mne-nirs bug does appear to be fixed by this revert PR. If I add this line to debug:

diff --git a/mne_bids/stats.py b/mne_bids/stats.py
index aeed07b3..df1bcad3 100644
--- a/mne_bids/stats.py
+++ b/mne_bids/stats.py
@@ -71,6 +71,7 @@ def count_events(root_or_path, datatype="auto"):
 
     bids_path.update(datatype=datatype)
 
+    print(f"{repr(bids_path).replace('\n',' ')} ({len(bids_path.match())} files)")
     tasks = sorted(set([bp.task for bp in bids_path.match()]))
 
     all_counts = []

on this branch I get:

BIDSPath( root: /Users/larsoner/mne_data/fNIRS-motor-group datatype: nirs basename: events.tsv) (5 files)

Whereas main I see there is no .match for the tasks:

BIDSPath( root: /Users/larsoner/mne_data/fNIRS-motor-group datatype: nirs basename: events.tsv) (0 files)

And there are files like:

$ ls /Users/larsoner/mne_data/fNIRS-motor-group/*/*/*_events.tsv 
/Users/larsoner/mne_data/fNIRS-motor-group/sub-01/nirs/sub-01_task-tapping_events.tsv
/Users/larsoner/mne_data/fNIRS-motor-group/sub-02/nirs/sub-02_task-tapping_events.tsv
/Users/larsoner/mne_data/fNIRS-motor-group/sub-03/nirs/sub-03_task-tapping_events.tsv
/Users/larsoner/mne_data/fNIRS-motor-group/sub-04/nirs/sub-04_task-tapping_events.tsv
/Users/larsoner/mne_data/fNIRS-motor-group/sub-05/nirs/sub-05_task-tapping_events.tsv

maybe this will help a test to be added?

[sappelhoff]

Thanks for looking into this @larsoner and @drammock.

I unfortunately do not have the time to properly help debugging this, so if it is urgent for you, or you deem it wise, please go ahead with reverting the change.

@waldie11 can maybe then pick it up again in the near future to resolve the failures that our tests and my review did not pick up on.

[larsoner]

I wrote a little test that fails on main but passes on this PR based on what I wrote above ☝️ I'll wait until mne-tools/mne-python#13129 lands, which should fix CIs, then restart CIs here. Then I'll push that test. Then I think we should merge.

[larsoner]

Okay I think I actually found the bug, I'll open a PR. I think there was just a missing * 🤦