[MRG] Add motion to BIDSPath #1430

JuliusWelzel · 2025-08-25T07:13:04Z

PR Description

This PR adds motion as an allowed datatype for the BIDSPath with relevant changes in the config.py in a693a4e.

Closes #1145.

Merge checklist

Maintainer, please confirm the following before merging.
If applicable:

All comments are resolved
This is not your own PR
All CIs are happy
PR title starts with [MRG]
whats_new.rst is updated
New contributors have been added to CITATION.cff
PR description includes phrase "closes <#issue-number>"

…nd duration columns

for more information, see https://pre-commit.ci

JuliusWelzel · 2025-08-27T06:49:00Z

The CI's fail for two example scripts anonymize_dataset.py and bidspath.py (Error here).

If I run the same scripts locally, I cannot recreate the same error. @sappelhoff, maybe you have an idea how to solve this?

scott-huberty · 2025-08-27T15:02:32Z

@JuliusWelzel If I checkout your branch locally, I do get the same error that the CI's are hitting:

RuntimeError: Found more than one matching data file for the requested recording. While searching:
    BIDSPath(
    root: /Users/scotterik/devel/repos/mne-bids/mne_bids/tests/data/tiny_bids
    datatype: eeg
    basename: sub-01_ses-eeg)
Found 5 paths:
    /Users/scotterik/devel/repos/mne-bids/mne_bids/tests/data/tiny_bids/sub-01/ses-eeg/eeg/sub-01_ses-eeg_electrodes.tsv
    /Users/scotterik/devel/repos/mne-bids/mne_bids/tests/data/tiny_bids/sub-01/ses-eeg/eeg/sub-01_ses-eeg_space-CapTrak_electrodes.tsv
    /Users/scotterik/devel/repos/mne-bids/mne_bids/tests/data/tiny_bids/sub-01/ses-eeg/eeg/sub-01_ses-eeg_task-rest_channels.tsv
    /Users/scotterik/devel/repos/mne-bids/mne_bids/tests/data/tiny_bids/sub-01/ses-eeg/eeg/sub-01_ses-eeg_task-rest_eeg.vhdr
    /Users/scotterik/devel/repos/mne-bids/mne_bids/tests/data/tiny_bids/sub-01/ses-eeg/eeg/sub-01_ses-eeg_task-rest_events.tsv

I think I see the problem.

scott-huberty · 2025-08-27T15:08:39Z

mne_bids/config.py

+allowed_extensions_motion = [
+    ".tsv",  # Tab-separated values
+]
+


This seems to be the line causing the issue, which makes sense because the EEG BIDS datasets have sidecar files that are just text files. So adding .tsv to the allowed datafile extensions seems to be confusing some of mne_bids internal functions..

Motion data might be the first modality with data that are stored in simple text files that we are trying to add into MNE-BIDS, which is why we are hitting this problem now. But I know that Eye-tracking BIDS will have the same issue.. So supporting these data might require more refactoring of the codebase.

As a grumpy sidenote, I never understood why BIDS encourages storing some modalities of data in text files... !

without having looked into it too deeply, that could be something that needs refactoring, yes.

drammock

couple minor comments. I haven't yet tried to debug the CI error

drammock · 2025-08-27T15:35:26Z

mne_bids/config.py

    "description",
    "suffix",
    "extension",
+    "tracking-system",


this is hyphen-separated, but below in ALLOWED_PATH_ENTITIES_SHORT it's underscore_separated. Is that intentional?

drammock · 2025-08-27T15:35:38Z

mne_bids/config.py

    "physio",
    "stim",  # behavioral
    "nirs",
+    "motion",  # motion


redundant comment

Suggested change

"motion", # motion

"motion",

drammock · 2025-08-27T15:48:13Z

49b5c02 (#1430) should fix the tests. Honestly I'm not sure it's the right fix, but to me the way it was before was buggy / coincidentally working. Adding motion revealed a bug IMO. But we should check with @hoechenberger or @sappelhoff, whether my fix is the right one.

sappelhoff · 2025-09-07T11:57:56Z

Honestly I'm not sure it's the right fix, but to me the way it was before was buggy / coincidentally working.

the fix looks good to me

…sponding change in 'ALLOWED_PATH_ENTITIES_SHORT'

… test_get_entities_from_fname_errors

for more information, see https://pre-commit.ci

…LLOWED_PATH_ENTITIES and ALLOWED_PATH_ENTITIES_SHORT

…and add it to valid entities

for more information, see https://pre-commit.ci

codecov · 2025-09-08T08:02:25Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.43%. Comparing base (a478527) to head (c235ca2).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1430   +/-   ##
=======================================
  Coverage   97.42%   97.43%           
=======================================
  Files          40       40           
  Lines        9136     9147   +11     
=======================================
+ Hits         8901     8912   +11     
  Misses        235      235

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

JuliusWelzel · 2025-09-08T10:31:11Z

Hi,

thanks for the help with fixing the tests. All CI's are passing now. One question from my side:
The tracksys path entity was introduced with Motion BIDS. In 0373d18 I changed all tracking-system to tracking_system. One reason for this was that the BIDSPath.__init__ function cannot take tracking-system with a hyphen. Another option would be that I simply rename it to trackingsystem to follow the current format. Any thoughts on this?

drammock · 2025-09-08T11:38:11Z

Another option would be that I simply rename it to trackingsystem to follow the current format. Any thoughts on this?

Underscore seems ok to me.

sappelhoff

@JuliusWelzel could you please also add a what's new entry and resolve the conflicts?

for more information, see https://pre-commit.ci

sappelhoff

Cool, LGTM! Thanks @JuliusWelzel.

If this is doing what you need for mne-bids to be helpful for motion data, then I think this can be merged as is.

JuliusWelzel · 2025-09-29T19:18:45Z

Thanks @sappelhoff.

The changes would now allow me to use mne-bids to handle the path to motion, but not write motion data.
Once the class is created, I would update it for each file and "manually" write my data in BIDS-like manner into the dir:

import numpy as np
from mne_bids import BIDSPath

# define motion-bids relevant information
subject_id = "01"
session = "01"
task = "walking"
tracking_system = "imu1"

# initialize BIDSPath for motion data
motion_raw_bids_path = BIDSPath(subject=subject_id, task=task, datatype='motion', tracking_system=tracking_system, root=bids_root)
print(motion_raw_bids_path)
motion_raw_bids_path.mkdir()

# generate random data for 9 markers and 10 seconds of data at 200Hz srate
raw_motion_data = np.random.rand(9, 10 * 200) # 9 channels, 10 seconds, 200 Hz

#store as tsv without headers
np.savetxt(str(motion_raw_bids_path), raw_motion_data, delimiter='\t', header='', comments='')

# update path for motion.json
motion_json_bids_path = motion_raw_bids_path.update(extension='.json')
print(motion_json_bids_path)

# update path for channels
motion_channels_bids_path = motion_raw_bids_path.update(suffix='channels', extension='.tsv')
print(motion_channels_bids_path)

As long as MNE does not want/allow motion data the current approach is already helpful. However, I am open to extend the write_raw_bids function to allow motion data, but would need a little support.

If you think this the current approach is to much of an edge case and not relevant to Brain Imaging Data, I am happy not to merge :)

sappelhoff · 2025-09-29T19:24:18Z

Thanks for the explanation @JuliusWelzel I am happy with how you aim to use mne-bids for motion data. And will merge this, thanks!

Looking forward to future enhancements (to mne or mne-bids) for motion data compatibility. 🚀

* fixups from #1430 * fix wrong orig_unit sometimes written to channels.tsv * fix doctest (bad syntax; add verbose; avoid np types) * add curryreader to full and test deps * importorskip

JuliusWelzel and others added 18 commits May 27, 2024 13:11

[ADD] describing fields in event.json file for the REQUIRED onset a…

5832654

…nd duration columns

[pre-commit.ci] auto fixes from pre-commit.com hooks

9860744

for more information, see https://pre-commit.ci

fix line length <88

b1ef7fe

[pre-commit.ci] auto fixes from pre-commit.com hooks

208885b

for more information, see https://pre-commit.ci

remove column position from description

8895caf

Merge branch 'main' into main

74566f2

only provide description and onset for the json

61030eb

refine description of sample

3e31c6b

[pre-commit.ci] auto fixes from pre-commit.com hooks

6ade344

for more information, see https://pre-commit.ci

satisfy contributing guidelines

64ec71a

Merge branch 'main' of https://github.com/JuliusWelzel/mne-bids

eb984b7

[pre-commit.ci] auto fixes from pre-commit.com hooks

37faafc

for more information, see https://pre-commit.ci

move author to correct position in CITATION.cff

3a37466

[ADD] adapt the config file to include motion data type

a693a4e

Merge branch 'main' of https://github.com/JuliusWelzel/mne-bids

5026015

[pre-commit.ci] auto fixes from pre-commit.com hooks

31fe43c

for more information, see https://pre-commit.ci

[ADD] include tracking_system in ENTITY_VALUE_TYPE and BIDSPath class

5780de7

Merge branch 'main' of https://github.com/JuliusWelzel/mne-bids

1bbc42b

scott-huberty reviewed Aug 27, 2025

View reviewed changes

drammock reviewed Aug 27, 2025

View reviewed changes

fix tests

49b5c02

JuliusWelzel and others added 5 commits September 8, 2025 08:42

[FIX] update spelling of 'tracking-system' in test_path.py with corre…

2bcb5b2

…sponding change in 'ALLOWED_PATH_ENTITIES_SHORT'

[ADD] include 'tracking-system' in the list of valid entity values in…

c5a18a1

… test_get_entities_from_fname_errors

Merge branch 'main' into main

973f536

[pre-commit.ci] auto fixes from pre-commit.com hooks

60e7661

for more information, see https://pre-commit.ci

[FIX] correct spelling of 'tracking-system' to 'tracking_system' in A…

0373d18

…LLOWED_PATH_ENTITIES and ALLOWED_PATH_ENTITIES_SHORT

Merge branch 'main' of https://github.com/JuliusWelzel/mne-bids

46d1e63

JuliusWelzel changed the title ~~Add motion to BIDSPath~~ [MRG] Add motion to BIDSPath Sep 8, 2025

JuliusWelzel and others added 2 commits September 8, 2025 09:48

[FIX] update entity name from 'tracking-system' to 'tracking_system' …

c581f89

…and add it to valid entities

[pre-commit.ci] auto fixes from pre-commit.com hooks

8f1ff0e

for more information, see https://pre-commit.ci

Merge branch 'main' into main

be2b713

sappelhoff reviewed Sep 25, 2025

View reviewed changes

JuliusWelzel and others added 9 commits September 28, 2025 20:54

Merge branch 'main' into main

734b427

fix whats_new.rst

b508ff4

FIX: tracking_system update in BIDSPath class

14069fb

FIX: correct path reference for BIDSPath initialization in whats_new.rst

35ab863

FEAT: add tracking_system parameter to _filter_fnames function

8849b22

[pre-commit.ci] auto fixes from pre-commit.com hooks

7124242

for more information, see https://pre-commit.ci

fix whats_new.rst to ref BIDSPath

04bbe04

Merge branch 'main' of https://github.com/JuliusWelzel/mne-bids

2b4a3d2

fix whats_new.rst to ref BIDSPath

c235ca2

sappelhoff approved these changes Sep 28, 2025

View reviewed changes

sappelhoff merged commit 2ccfdf1 into mne-tools:main Sep 29, 2025
24 checks passed

drammock added a commit to drammock/mne-bids that referenced this pull request Nov 10, 2025

fix doctest to reflect tracksys (leftover from mne-tools#1430)

0aab19e

drammock added a commit to drammock/mne-bids that referenced this pull request Nov 10, 2025

fixups from mne-tools#1430

494e825

drammock mentioned this pull request Nov 10, 2025

fixes related to doctests #1478

Closed

7 tasks

drammock added a commit to drammock/mne-bids that referenced this pull request Nov 11, 2025

fixups from mne-tools#1430

57326d2

drammock added a commit to drammock/mne-bids that referenced this pull request Nov 13, 2025

fixups from mne-tools#1430

17cc983

[MRG] Add motion to BIDSPath #1430

[MRG] Add motion to BIDSPath #1430

Uh oh!

Conversation

JuliusWelzel commented Aug 25, 2025 • edited by sappelhoff Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Description

Merge checklist

Uh oh!

JuliusWelzel commented Aug 27, 2025

Uh oh!

scott-huberty commented Aug 27, 2025

Uh oh!

scott-huberty Aug 27, 2025 • 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!

scott-huberty Aug 27, 2025

Choose a reason for hiding this comment

Uh oh!

sappelhoff Sep 7, 2025

Choose a reason for hiding this comment

Uh oh!

drammock left a comment

Choose a reason for hiding this comment

Uh oh!

drammock Aug 27, 2025

Choose a reason for hiding this comment

Uh oh!

drammock Aug 27, 2025

Choose a reason for hiding this comment

Uh oh!

drammock commented Aug 27, 2025

Uh oh!

sappelhoff commented Sep 7, 2025

Uh oh!

codecov bot commented Sep 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

JuliusWelzel commented Sep 8, 2025

Uh oh!

drammock commented Sep 8, 2025

Uh oh!

sappelhoff left a comment

Choose a reason for hiding this comment

Uh oh!

sappelhoff left a comment

Choose a reason for hiding this comment

Uh oh!

JuliusWelzel commented Sep 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sappelhoff commented Sep 29, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

JuliusWelzel commented Aug 25, 2025 •

edited by sappelhoff

Loading

scott-huberty Aug 27, 2025 •

edited

Loading

codecov bot commented Sep 8, 2025 •

edited

Loading

JuliusWelzel commented Sep 29, 2025 •

edited

Loading