-
Notifications
You must be signed in to change notification settings - Fork 161
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 recording naming conventions #480
[FIX] clarify MEG empty-room recording naming conventions #480
Conversation
Maybe to allow omitting them entirely? And the intention was not to allow arbitrary values? |
But they cannot be ommitted. Task label is a required label. |
If you ask me, SHOULD is pretty much OPTIONAL because validator does not check for it :) 30% of the spec is checked in the validator. |
That's true. But |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've made a few suggestions, feel free to apply or ignore them :)
49bf001
to
5d67f5e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made some suggestions regarding phrasing and markup, feel free to apply to ones you like and leave out the others.
thanks @hoechenberger :) |
Thanks for your reviews @jasmainak @robertoostenveld and @hoechenberger before I merge I just want to make sure you all saw the final version and approve of it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good, thanks!
sorry for the last minute comments! 🙈 |
Co-authored-by: Richard Höchenberger <[email protected]>
Co-authored-by: Richard Höchenberger <[email protected]>
Co-authored-by: Richard Höchenberger <[email protected]>
e4c04f8
to
975e66f
Compare
@robertoostenveld @jasmainak we got an answer on MRI phantoms: #480 (comment) Also, I voiced my opinion here: #480 (comment) would be cool if you could take a minute to respond, so that we can move forward with this PR. |
I am happy with the modifications as proposed in this PR, as they align with what I recall from the in-person discussions we had about this in the preparation of BIDS-MEG. But I would also be fine to leave out the 2nd strategy ( Furthermore, I think that clarifications to the specification are possibly not always the best discussed as changes to the specification text. We have other channels (google group, neurostars, BIDS starterkit, brainhack mattermost) that I think are more appropriate for clarification questions, for documenting recommendations and use-cases, and for discussion things like those happening now here. |
Okay, I'll then strip down the proposed changes heavily so that the way emptyroom is currently described in the spec will essentially remain as is. I'll take the other things we talked about (re: emptyroom) to a page in the starter kit Thanks for your reviews all, I'll ping once more when I finished what I just said I'll do. |
I rewrote the changes and I think they are now mergeable. I am not introducing any new notions (anymore), but merely:
|
this looks good to me. So now you are essentially just adding clarifying language to directly address @hoechenberger 's issue without mixing it with other issues. If there are new use cases (= data or code) that suggest another strategy is needed to store empty room files (as explained by @robertoostenveld ), I'd be happy to discuss this in a separate issue! |
Thanks for the discussion and reviews everybody. The section is a tiny bit better (more understandable and consistent) now :-) |
this section was unclear, see mne-tools/mne-bids#421 (comment)
This PR aims at improving / clarifying the used language and examples.
I am wondering, why
noise
as a task name and date as a session label have only "SHOULD", and not "MUST" definitions 🤔 anyone got a clue?