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

[MISC] Restructure MEG empty room example texts #1677

Merged
merged 6 commits into from
Feb 5, 2024
Merged

Conversation

guiomar
Copy link
Collaborator

@guiomar guiomar commented Jan 8, 2024


Edit: Removed boilerplate.

Copy link

codecov bot commented Jan 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (master@a73a310). Click here to learn what that means.

❗ Current head 0f3d0a4 differs from pull request most recent head 441fa10. Consider uploading reports for the commit 441fa10 to get more accurate results

Additional details and impacted files
@@            Coverage Diff            @@
##             master    #1677   +/-   ##
=========================================
  Coverage          ?   87.97%           
=========================================
  Files             ?       16           
  Lines             ?     1356           
  Branches          ?        0           
=========================================
  Hits              ?     1193           
  Misses            ?      163           
  Partials          ?        0           

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

@effigies effigies changed the title Update magnetoencephalography.md [ENH] Clarify MEG empty room text, add links to examples Jan 8, 2024
Comment on lines 512 to 518
In the case of empty-room recordings being associated with multiple subjects and/or sessions (see [Example 1](#example-1)), it is RECOMMENDED to store the empty-room recording inside a subject directory named `sub-emptyroom`.
If a [`session-<label>`](../appendices/entities.md#ses) entity is present, its label SHOULD be the date of the empty-room recording in the format `YYYYMMDD`, that is `ses-YYYYMMDD`.
The `scans.tsv` file containing the date and time of the acquisition SHOULD also be included.
The rationale is that this naming scheme will allow users to easily retrieve the empty-room recording that best matches a particular experimental session, based on date and time of the recording.
It should be possible to query empty-room recordings just like usual subject recordings, hence all metadata sidecar files (such as the `channels.tsv`) file SHOULD be present as well.

In the case of empty-room recordings being collected for the individual experimental session, it is recommended to store the empty-room recording along with that subject and session.
In the case of empty-room recordings being collected for the individual experimental session (see [Example 2](#example-2)), it is RECOMMENDED to store the empty-room recording along with that subject and session.
Copy link
Collaborator

Choose a reason for hiding this comment

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

These additions feel redundant with L509/L510:

image

If keeping the examples close to the more detailed descriptions makes more sense, maybe we should remove these links from L509/L510 and reword that to be a bit smoother without the examples?

Copy link
Member

Choose a reason for hiding this comment

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

I am impartial as to whether that makes more sense, but I agree that we should not introduce redundancy here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I got a couple of questions here, so I decided to reinforce the links to the examples. But maybe not that necessary.
What about instead of using "(see Example 1)" as in lines L509/L510, say just "(Example 1)". People often read fast, and a bit of redundancy sometimescan may help.

Co-authored-by: Julia Guiomar Niso Galán <guiomar.niso@ctb.upm.es>
@@ -546,7 +546,7 @@ A guide for using macros can be found at

### Example 2

One recording per session, stored within the session directory.
One empty-room recording per each participant's session, stored within the session directory.
Copy link
Member

Choose a reason for hiding this comment

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

in BIDS, a session is always "per participant" (that is, you would not have two participants listed in the same session directory)

Suggested change
One empty-room recording per each participant's session, stored within the session directory.
One empty-room recording per session, stored within the session directory.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was to clarify that in the other example (1) you can store under one session the noise recording to be used for multiple participants, but it's true it is redundant. How to store the emptyroom recordings is the thing I typically get the more questions about, as it is very MEG specific not applicable to other modalities, that's why I might have been over redundant in many places :)

Suggested change
One empty-room recording per each participant's session, stored within the session directory.
One empty-room recording per session, stored within the participant's session directory.

Copy link
Member

Choose a reason for hiding this comment

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

okay, let me try to push a commit and we can see whether you think that's an acceptable improvement.

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.

To avoid redundancies, I have pulled the example descriptions to be under the example subheadings. What do you think @guiomar @effigies?

@effigies
Copy link
Collaborator

This is fine. I'm not 100% in favor of including spec language likes SHOULDs and MUSTs in examples, but it doesn't change the meaning of the spec, so if this helps people find the information they need, fine.

@sappelhoff sappelhoff changed the title [ENH] Clarify MEG empty room text, add links to examples [ENH] Restructure MEG empty room example texts Feb 5, 2024
@sappelhoff sappelhoff changed the title [ENH] Restructure MEG empty room example texts [MISC] Restructure MEG empty room example texts Feb 5, 2024
@sappelhoff sappelhoff merged commit 365f321 into master Feb 5, 2024
23 of 24 checks passed
@sappelhoff sappelhoff deleted the guiomar-patch-1 branch February 5, 2024 15:51
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 this pull request may close these issues.

3 participants