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

Make use of AssociatedEmptyRoom field in *_meg.json #493

Closed
hoechenberger opened this issue Aug 7, 2020 · 13 comments · Fixed by #795
Closed

Make use of AssociatedEmptyRoom field in *_meg.json #493

hoechenberger opened this issue Aug 7, 2020 · 13 comments · Fixed by #795
Assignees
Milestone

Comments

@hoechenberger
Copy link
Member

I don't know how I could have missed this all this time, but the *_meg.json sidecar files support a field AssociatedEmptyRoom:

RECOMMENDED. Relative path in BIDS folder structure to empty-room file associated with the subject’s MEG recording. The path needs to use forward slashes instead of backward slashes (e.g. sub-emptyroom/ses-/meg/sub-emptyroom_ses-_task-noise_run-_meg.ds).

I think we should make use of this.

By default, write_raw_bids() should try to retrieve the matching empty-room recording via mne_bids.get_matched_empty_room() (which would imply that empty-room data need to be added to the BIDS folder first, before adding participants' recordings).

To explicitly specify an empty-room recording, write_raw_bids should gain a kwarg emptyroom_session=None, which would accept a string with the name of the associated empty-room session. This can then be used by write_raw_bids() to construct the relative path to the requested empty-room recording file.

In both cases (default behavior where we infer the empty-room recording, and in cases where users explicitly request an empty-room recording), write_raw_bids() would add the correct AssociatedEmptyRoom to the JSON sidecar file.

Only question that remains is what to do in cases where empty-room recordings are missing / how to design the API such that this case would be covered too.

get_matched_emptyroom() would be updated to first check if there's an AssociatedEmptyRoom entry in the sidecar; and only if not, it would actually start looking at the file names in the dataset.

Would like to hear your thoughts on that!

@jasmainak
Copy link
Member

can you check if any datasets actually use this field? I think it's a fairly new field but I'm not sure

@hoechenberger
Copy link
Member Author

can you check if any datasets actually use this field?

I could, but is that relevant? :) Just suggesting to add it for writing, and respect it when reading, if it's present at all

@jasmainak
Copy link
Member

I could, but is that relevant? :)

It will help you understand how to write the code. For example, looking here tells me that it's not always in a sidecar associated with a file. It's always good to ground oneself in reality ;-)

@hoechenberger
Copy link
Member Author

Sorry, I cannot follow you… the file you linked to has an entry:

 "AssociatedEmptyRoom": "sub-emptyroom/ses-20090409/meg/sub-emptyroom_ses-20090409_task-noise_meg.fif.gz"

and this file, in fact, does exist in the dataset. 🧐

@jasmainak
Copy link
Member

but which raw file is it for?

@hoechenberger
Copy link
Member Author

Ooooh now I see what you mean! I didn't even know this was allowed in BIDS …

@jasmainak
Copy link
Member

welcome to BIDS inheritance principle :)

the specification is too complex sometimes.

@hoechenberger
Copy link
Member Author

Still: when writing, we could write this attribute -- we'll be in full control of where to put it.

Reading -- needs to be discussed.

@hoechenberger
Copy link
Member Author

the specification is too complex sometimes.

sometimes .... *cough, cough*
😭🙈

@jasmainak
Copy link
Member

hahaha.

I believe for ephys, mne-bids is the most comprehensive option out there :) I agree we could have an option for writing. Go for it if you like. For reading, we don't deal with the inheritance principle currently, so I think that can be safely ignored.

@sappelhoff
Copy link
Member

welcome to BIDS inheritance principle :)

place to complain/discuss about this --> bids-standard/bids-2-devel#36

I see two problems with the inheritance principle:

  1. it's not very rigidly defined, see: Inheritance principle: clarify the procedure of which files would be considered bids-standard/bids-specification#102
  2. the validator does not have sufficient coverage to check that the principle is reasonably used (and warn or error otherwise)

@hoechenberger
Copy link
Member Author

hoechenberger commented Aug 8, 2020

I see two problems with the inheritance principle:

My main concern is that there is no central authority one can query when looking at a subject's individual data. I feel that ideally, either everything would be specified in the BIDS root – or on the same level as participants' data files. And not split across different levels, including inheritance… Ugh, what a standard…

@jasmainak
Copy link
Member

My main concern is that there is no central authority one can query when looking at a subject's individual data.

higher levels of hierarchy do take precedence ...

Ugh, what a standard…

can you please complain on the bids-specification github and/or forums? :-P Sometimes I feel I'm the only one shouting ;-)

@hoechenberger hoechenberger added this to the 0.8 milestone Mar 12, 2021
@hoechenberger hoechenberger self-assigned this May 6, 2021
hoechenberger added a commit to hoechenberger/mne-bids that referenced this issue May 6, 2021
agramfort pushed a commit that referenced this issue May 7, 2021
* Make use of AssociatedEmptyRoom field in *_meg.json

Fixes #493

* Remove new property

* flake

* Apply suggestions from code review

* Fix

* Missing whitespace

* Update changelog

* Try to fix Windows

* Another attempt to fix Windows

* Hopefully fix Windows tests

* Reformat
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

Successfully merging a pull request may close this issue.

3 participants