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

support writing empty room as task-noise in subject's directory (not in sub-emptyroom) #1374

Open
drammock opened this issue Feb 19, 2025 · 4 comments

Comments

@drammock
Copy link
Member

I actually thought passing empty_room would store it in the subject's directory. We should definitely support this. In fact, I'd suggest this should become our default behavior…

Originally posted by @hoechenberger in #1373

I think we should support both strategies supported in the BIDS spec and make it easy for users to adopt either of them.

Originally posted by @sappelhoff in #1373

@drammock
Copy link
Member Author

+1 to supporting both strategies. For backwards compatibility the default should be writing into sub-emptyroom IMO. I'd be open to changing the default via deprecation, but I don't have a clear sense of which use case is more prevalent (storing in sub-emptyroom is more efficient if multiple subjects' data are collected on a single day, all sharing the same ERM file)

@hoechenberger
Copy link
Member

When working with @SophieHerbst we commonly had one ER recording per day. Saving it as sub-emptyroom, such that it could be "shared" between all participants measured in that day, saved significant amounts of disk space and data transfer time (network-based storage!). I also remember that converting the dataset to BIDS in the first place also took quite a toll on network I/O capacity.

I'd argue today that, considering that common ER recordings are rather short, storage is cheap, and processing can be parallelized, the only bottleneck remaining is network I/O.

Back then we didn't have a caching implementation in MNE-BIDS-Pipeline; now we do and avoid unnecessary processing (data retrieval, computations, storage) upon re-run.

The way MNE-BIDS-Pipeline worked back then (and works still today), if I'm not mistaken, is to process ER recordings for each participant separately in any case, even if there is an sub-emptyroom subject and ER recordings are "shared" between subjects -- Is this correct? In that case, effects on compute and storage requirements for the created artifacts would be zero.

... just to share a few thoughts that come to my mind; not really structured or complete, but I wanted to note them down anyway 😃

@hoechenberger
Copy link
Member

hoechenberger commented Feb 20, 2025

Proposal: introduce a new parameter to write_raw_bids(), for example:

empty_room_storage: "individual" | "global"

@drammock
Copy link
Member Author

drammock commented Feb 20, 2025

The way MNE-BIDS-Pipeline worked back then (and works still today), if I'm not mistaken, is to process ER recordings for each participant separately in any case, even if there is an sub-emptyroom subject and ER recordings are "shared" between subjects -- Is this correct?

you're correct. "shared" ERMs in sub-emptyroom end up in derivatives/mne_bids_pipeline/sub-*/ses-*/meg/*task-noise_proc-*.fif. So there's really quite a small effect on storage size of the initial datset only (not the derivatives), if you're using MNE-BIDS-pipeline. But we are talking about a change to MNE-BIDS, not MNE-BIDS-pipeline, and there are presumably other downstream packages/pipelines/scripts that might break if we suddenly start writing ERMs to the subject folder instead of sub-emptyroom. So I'm still in favor of adding the option right away, then doing a deprecation cycle for the changing of its default.

empty_room_storage: "individual" | "global"

since there are only 2 legal options in the BIDS standard, I'd be more inclined toward a boolean option like

use_empty_room_subject: bool = True
# or
store_empty_room_as_noise_task: bool = False

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

No branches or pull requests

2 participants