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

[FIX] Clarify MEG empty-room recommendations #1125

Merged
merged 13 commits into from
Jul 20, 2022

Conversation

robertoostenveld
Copy link
Collaborator

@robertoostenveld robertoostenveld commented Jun 14, 2022

@mcvinding noted to me that the recommendation for empty-room MEG recordings was confusing, as it confounds two practices.

Some labs record an empty-room recording every morning to be shared, that is then to be used for all subjects/sessions recorded during that day.

Other labs record an empty-room recording prior to each experimental session, those are not shared.

The text specified that it is "RECOMMENDED to perform an empty-room recording for each experimental session" (i.e., non -shared) and subsequently it explained how to organize the daily shared empty-room recordings in a sub-emptyroom/ses-YYYYMMDD structure.

The updated text splits the two and does not prescribe the recommended ses-YYYYMMDD practice for empty-room recordings done prior to each experimental session, as those are more logically shared with the actual subject and session.

@guiomar
Copy link
Collaborator

guiomar commented Jun 14, 2022

I'm very happy that we finally adopt this practice. While creating the specs many years ago this idea (i.e. having the associated empty rooms inside each subjects folder, when recorded especifically for each subject/session) was discarted, and I felt this produced a folder structure and code to match these files that was unnecessarily complicated.

For example, we experienced that while organizing the OMEGA repository with hundreds of participants many of them with multiple sessions, and each of them with a emptyroom recording that was dumped into the emptyroom subject mixing them up and making it less intuitive.

@ftadel @Moo-Marc @rcassani I tag you here in case we finally merge this PR, so you are aware for the brainstorm BIDS importer and reorganization of the OMEGA dataset.

I have added a exampe. 
- Shall it be named task-noise, or task-emptyroom? 
- Shall it be named sub-control01 (as it will be past of this subject) or sub-emptyroom (eventhough it's inside sub-control01)?

Please edit as you see
Copy link
Contributor

@Moo-Marc Moo-Marc left a comment

Choose a reason for hiding this comment

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

Hi all,

I also agree with clarifying the two approaches. @guiomar, I had already decided to switch OMEGA to putting noise recordings into individual subject folders, after this discussion. Regarding your second example, yes I think it should be sub-control01 and task-noise.

Do we want to comment on the possibility of mixing the two approaches? It could be useful in a large repository mixing data from various sources, or if a lab does both approaches. The daily recordings could be used by a lab tech, and the subject-specific ones for analysis. If we allow it, we could merge the two examples.

@guiomar
Copy link
Collaborator

guiomar commented Jun 14, 2022

Yes @Moo-Marc I think it would be a good idea that the two approaches could be merged together

Copy link
Contributor

@alexrockhill alexrockhill left a comment

Choose a reason for hiding this comment

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

This seems very reasonable +1 from me.

If you have 100 subjects and 50 empty room sessions with 2 subjects collected on each day, is there a way not to duplicate each empty room session twice? Maybe to use the Example 1 method but add an empty_ses tag to the task data in a sidecar (rest in the example)? Or is the best solution to just store the duplicate? And, if so, can you make a symbolic link and still have it pass BIDS? As I'm writing this, I'm thinking that answers to these questions might be nice to have in the specification also.

@hoechenberger
Copy link
Collaborator

hoechenberger commented Jun 14, 2022

Yes @Moo-Marc I think it would be a good idea that the two approaches could be merged together

But then we'd have an ambiguity if both are present. Which one should a software implementation load?

@hoechenberger
Copy link
Collaborator

hoechenberger commented Jun 14, 2022

If you have 100 subjects and 50 empty room sessions with 2 subjects collected on each day, is there a way not to duplicate each empty room session twice? Maybe to use the Example 1 method but add an empty_ses tag to the task data in a sidecar (rest in the example)? Or is the best solution to just store the duplicate?

At NeuroSpin, we've done it such that when you convert the freshly recorded data to BIDS, basically everything would be symlinks, so you could "duplicate" stuff a million times if you wanted to and still wouldn't run out of disk space :) We use the symlink parameter of MNE-BIDS's write_raw_bids().

Also makes the conversion much faster ;)

@alexrockhill
Copy link
Contributor

If you have 100 subjects and 50 empty room sessions with 2 subjects collected on each day, is there a way not to duplicate each empty room session twice? Maybe to use the Example 1 method but add an empty_ses tag to the task data in a sidecar (rest in the example)? Or is the best solution to just store the duplicate?

At NeuroSpin, we've done it such that when you convert the freshly recorded data to BIDS, basically everything would be symlinks, so you could "duplicate" stuff a million times if you wanted to and still wouldn't run out of disk space :) We use the symlink parameter of MNE-BIDS's write_raw_bids().

Also makes the conversion much faster ;)

That sounds nice and related to this. I know symlinks are probably mentioned other places in the BIDS spec but my 2 cents would be that it's nice having the recommended strategy all in the same place.

@robertoostenveld
Copy link
Collaborator Author

My proposal is to keep this pull request as just a small clarification of the current text without any extensions to it. It is possible to come up with further improvements for empty room data, but I think that would better be a separate PR that follows this.

@hoechenberger
Copy link
Collaborator

My proposal is to keep this pull request as just a small clarification of the current text without any extensions to it. It is possible to come up with further improvements for empty room data, but I think that would better be a separate PR that follows this.

Absolutely, this was just a bit of an off-topic discussion :)

instead it is now a recommendation to associate the data to an empty-room recording.
- example 1: one per day in sub-emptyroom
- example 2: one per session, along with normal subject and session
Copy link
Member

@sappelhoff sappelhoff left a comment

Choose a reason for hiding this comment

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

I think this is a nice clarification. Thanks a lot!

@sappelhoff sappelhoff changed the title [FIX] - clarify MEG empty-room recommendation [FIX] Clarify MEG empty-room recommendations Jul 12, 2022
Copy link

@dnacombo dnacombo left a comment

Choose a reason for hiding this comment

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

No reference to the recommended "AssociatedEmptyRoom" field of the json sidecar?

@robertoostenveld
Copy link
Collaborator Author

No reference to the recommended "AssociatedEmptyRoom" field of the json sidecar?

There is now the sentence "In the context of BIDS it is RECOMMENDED to associate an empty-room recording to each experimental session". That would indeed work by specifying AssociatedEmptyRoom. But I don't think that we need to spell that out more explicitly.

Copy link
Collaborator

@hoechenberger hoechenberger left a comment

Choose a reason for hiding this comment

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

Two tiny suggestion re phrasing

robertoostenveld and others added 2 commits July 12, 2022 15:38
Co-authored-by: Richard Höchenberger <richard.hoechenberger@gmail.com>
Co-authored-by: Richard Höchenberger <richard.hoechenberger@gmail.com>
@hoechenberger
Copy link
Collaborator

No reference to the recommended "AssociatedEmptyRoom" field of the json sidecar?

There is now the sentence "In the context of BIDS it is RECOMMENDED to associate an empty-room recording to each experimental session". That would indeed work by specifying AssociatedEmptyRoom. But I don't think that we need to spell that out more explicitly.

Actually the same question as @dnacombo's popped up in my head while reading the draft.

I figured that we're essentially getting rid of AssociatedEmptyRoom here. Seems I was mistaken.

This confusion among @dnacombo and me seems to suggest the text still needs a bit of polishing..?

@hoechenberger
Copy link
Collaborator

hoechenberger commented Jul 12, 2022

... also because AssociatedEmptyRoom happens on a per-run and not on a per-session level ... at least that's how I added it to my metadata so far.

@robertoostenveld
Copy link
Collaborator Author

can you make a suggested edit to clarify it?

@hoechenberger
Copy link
Collaborator

can you make a suggested edit to clarify it?

I can try tonight, currently I'm only on my phone :)

@Moo-Marc
Copy link
Contributor

Thanks all, this looks good to me. Regarding the AssociatedEmptyRoom field, how about this:

"In the context of BIDS it is RECOMMENDED to associate an empty-room recording to each experimental session (see AssociatedEmptyRoom field in the *_meg.json sidecar file)".

Adjusting or removing the link as required - I'm not sure what's the convention.

@sappelhoff sappelhoff modified the milestones: 1.7.1, 1.8.0 Jul 13, 2022
Co-authored-by: Richard Höchenberger <richard.hoechenberger@gmail.com>
@sappelhoff sappelhoff merged commit 78e6fe2 into bids-standard:master Jul 20, 2022
@sappelhoff
Copy link
Member

Thanks everyone

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
MEG Magnetoencephalography
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants