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

[ENH] Extend BIDS for Motion data (BEP029) #981

Merged
merged 153 commits into from
Mar 17, 2023
Merged

[ENH] Extend BIDS for Motion data (BEP029) #981

merged 153 commits into from
Mar 17, 2023

Conversation

JuliusWelzel
Copy link
Collaborator

@JuliusWelzel JuliusWelzel commented Jan 20, 2022

@sappelhoff sappelhoff added the BEP label Jan 20, 2022
@JuliusWelzel
Copy link
Collaborator Author

I think the extra file for the BEP is working fine however, some checks still do not pass. Maybe @sappelhoff can help us out here? Thanks :)

@sappelhoff
Copy link
Member

sappelhoff commented Jan 28, 2022

@JuliusWelzel you can rebase your branch on master, or (easier) merge the commits from master into your branch. You are a few commits behind (6 commits, to be exact, see screenshot), and the issues of "linkchecker" have been fixed upstream :-)

image

PS: Let me know if you need help with that - I don't know how advanced your git knowledge is

@JuliusWelzel
Copy link
Collaborator Author

Worked, thank you @sappelhoff

@sappelhoff
Copy link
Member

This PR should also correct the text here:

All NIRS channels types MUST correspond to a [valid SNIRF data type](https://github.com/fNIRS/snirf/blob/master/snirf_specification.md#appendix).
Additional channels that are recorded simultaneously with the NIRS
device and stored in the same data file SHOULD be included as well.
However, additional channels that are simultaneously recorded with a different device
SHOULD be stored according to their appropriate modality specification.
For example, motion data that was simultaneously recorded with a different device should be specified
according to BEP029 and not according to the NIRS data type.
Whereas, if the motion data was acquired in with the NIRS device itself, it should be included here with the NIRS data.
Any of the channel types defined in other BIDS specification MAY be used here as well such as `ACCEL` or `MAGN`.
As several of these data types are commonly acquired using NIRS devices they are included as an example at the base of the table.
Note that upper-case is REQUIRED.

☝️ the mention of BEP029 should be adjusted to refer to the BIDS Motion modality instead.

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.

Comment on lines +162 to +163
Restricted keyword list for column `type` in alphabetic order.
Note that upper-case is REQUIRED:
Copy link
Member

Choose a reason for hiding this comment

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

not a blocker for merging this PR, but before release, we must clarify how channel types are shared between modalities. I have opened a new issue for this:

@sjeung
Copy link
Collaborator

sjeung commented Mar 8, 2023

Remi - Thanks for the useful note. Currently, the HED vocabulary for body parts is haphazard, but it should be straightforward to build out a structured (external) body part HED "library schema" from the DICOM BodyPart vocabulary https://dicom.nema.org/medical/dicom/current/output/chtml/part16/chapter_L.html#chapter_L you have pointed to. As the purpose of HED is to describe the nature of events occurring in neuroimaging experiments, the first needed terms are for external body parts - though as HED includes "data feature" events (alpha burst, seizures, etc.) which are addressed in the SCORE library schema (built by Dora Hermes and Tal Pal Attia from the neurophysiologists' agreed on SCORE vocabulary), 'brain parts' should also be included - I need to check to what extent they are already represented in SCORE... Note that the HED vocabulary is structured as a set of term hierarchies (thus formed as, technically, a 'heterarchy'). For body parts, this should be simple (for example, "leg" under the DICOM term "pelvis and lower extremities", etc.).

On Sat, Mar 4, 2023 at 4:51 AM Remi Gau @.> wrote: It would seem to me that the logical place to define hierarchies of body parts is in HED and not in BIDS per se. An anatomical library for HED is something I am thinking of and the hed working group has discussed. Within HED, the appropriate place for it might well be a subsidiary library schema that will attach to the base hed schema to avoid over bloating the base schema. Agreed that it would be best to rely on external "sources" for some sort of anatomical library. Ideally even better if kept in synch with the BodyPart examined metadata from DICOM as this is already the one used in BIDS for PET and microscopy. https://dicom.innolitics.com/ciods/cr-image/general-series/00180015 <https://urldefense.com/v3/https://dicom.innolitics.com/ciods/cr-image/general-series/00180015;!!Mih3wA!Hc0bHDP5WuxFQONK8FKdWfaNuIZ2vCRupiEQTEILNULUi45Bpa7wHBihk4k3cOg_wxkspFnmIMkZml19555NNkFM$> https://dicom.nema.org/medical/dicom/current/output/chtml/part16/chapter_L.html#chapter_L <https://urldefense.com/v3/https://dicom.nema.org/medical/dicom/current/output/chtml/part16/chapter_L.html*chapter_L;Iw!!Mih3wA!Hc0bHDP5WuxFQONK8FKdWfaNuIZ2vCRupiEQTEILNULUi45Bpa7wHBihk4k3cOg_wxkspFnmIMkZml1956z3dYo1$> — Reply to this email directly, view it on GitHub <https://urldefense.com/v3/https://github.com/bids-standard/bids-specification/pull/981*issuecomment-1454682475;Iw!!Mih3wA!Hc0bHDP5WuxFQONK8FKdWfaNuIZ2vCRupiEQTEILNULUi45Bpa7wHBihk4k3cOg_wxkspFnmIMkZml195xBsVNbb$>, or unsubscribe <https://urldefense.com/v3/https://github.com/notifications/unsubscribe-auth/AKN2SFWO4V3XXRJU532MAZDW2MGDTANCNFSM5MNBXIIA;!!Mih3wA!Hc0bHDP5WuxFQONK8FKdWfaNuIZ2vCRupiEQTEILNULUi45Bpa7wHBihk4k3cOg_wxkspFnmIMkZml195ykucWhS$> . You are receiving this because you were mentioned.Message ID: @.>
-- Scott Makeig, Research Scientist and Director, Swartz Center for Computational Neuroscience, Institute for Neural Computation, University of California San Diego, La Jolla CA 92093-0559, http://sccn.ucsd.edu/~scott

Thank you @smakeig for bringing up the points about and @Remi-Gau for the pointer, when we do get to work on the anatomical description project we will keep this in mind.

@sjeung
Copy link
Collaborator

sjeung commented Mar 8, 2023

This PR should also correct the text here:

All NIRS channels types MUST correspond to a [valid SNIRF data type](https://github.com/fNIRS/snirf/blob/master/snirf_specification.md#appendix).
Additional channels that are recorded simultaneously with the NIRS
device and stored in the same data file SHOULD be included as well.
However, additional channels that are simultaneously recorded with a different device
SHOULD be stored according to their appropriate modality specification.
For example, motion data that was simultaneously recorded with a different device should be specified
according to BEP029 and not according to the NIRS data type.
Whereas, if the motion data was acquired in with the NIRS device itself, it should be included here with the NIRS data.
Any of the channel types defined in other BIDS specification MAY be used here as well such as `ACCEL` or `MAGN`.
As several of these data types are commonly acquired using NIRS devices they are included as an example at the base of the table.
Note that upper-case is REQUIRED.

☝️ the mention of BEP029 should be adjusted to refer to the BIDS Motion modality instead.

Updated now : )

@smakeig
Copy link

smakeig commented Mar 8, 2023 via email

@sappelhoff sappelhoff dismissed their stale review March 15, 2023 14:37

critical aspects were addressed

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.

+1 for merging, and we can smooth out things (in particular the channel types situation) before a release with smaller, dedicated PRs.

@JuliusWelzel
Copy link
Collaborator Author

+1 for merging, and we can smooth out things (in particular the channel types situation) before a release with smaller, dedicated PRs.

I was just about to revert the added column int the channel type table, but happy to push that to the main branch

@sappelhoff
Copy link
Member

I was just about to revert the added column int the channel type table, but happy to push that to the main branch

let's wait with how the channel types discussion progresses 👍

@sappelhoff
Copy link
Member

I got some more approvals for this in the maintainers channel, so I will merge this now and we will keep working on it until the release of BIDS 1.9.0.

Congrats to @sjeung and @JuliusWelzel and your team for the work up to this point! 🎉

Looking forward to the release!

@sjeung
Copy link
Collaborator

sjeung commented Mar 17, 2023

I got some more approvals for this in the maintainers channel, so I will merge this now and we will keep working on it until the release of BIDS 1.9.0.

Congrats to @sjeung and @JuliusWelzel and your team for the work up to this point! 🎉

Looking forward to the release!

Thank you so much everyone! It's been awesome working on this extension. We will stay involved for further improvements and maintenance of the spec :)

@sappelhoff sappelhoff changed the title BEP029 Motion (spec) [ENH] Extend BIDS for Motion data (BEP029) Mar 17, 2023
@JuliusWelzel JuliusWelzel deleted the bep-029 branch March 17, 2023 09:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BEP enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.