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.

Sign up for GitHub

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

Jump to bottom

[ENH] Bep 009: Positron Emission Tomography #633

Merged
merged 184 commits into from
Apr 7, 2021
Merged

[ENH] Bep 009: Positron Emission Tomography #633

merged 184 commits into from
Apr 7, 2021

Conversation

melanieganz
Copy link
Contributor

@melanieganz melanieganz commented Oct 1, 2020 •
edited by sappelhoff
Loading

This PR incorporates BEP 009 - Positron Emission Tomography.

We have rebased bep009 with master before including all our BEP 009 changes into the bep009 branch and there are no direct conflicts. Hence, we would like to open this up to community comment.

Moderators: @melanieganz @mnoergaard

link to rendered draft: https://bids-specification.readthedocs.io/en/bep-009/

@melanieganz melanieganz added enhancement New feature or request PET BEP labels Oct 1, 2020
@effigies

This comment has been minimized.

Sign in to view
@mnoergaard

This comment has been minimized.

Sign in to view
@effigies
BEP-009 Initial PR
93c602f 
Squashed commit of the following:

commit 2d042c5
Merge: 2aa1395 77dd9b7
Author: melanieganz <[email protected]>
Date:   Thu Oct 1 09:43:48 2020 +0200

    Merge pull request #632 from mnoergaard/martin

    I am merging the changes for BEP 009 that were made in a private repository to be reflected in the BEP009 branch of the main spec.

commit 2aa1395
Merge: beaa985 2f0f61b
Author: melanieganz <[email protected]>
Date:   Thu Oct 1 09:34:52 2020 +0200

    Merge pull request #631 from bids-standard/master

    [MISC] Update of BEP009 with all master changes before PET BEP pull request

commit 77dd9b7
Author: mnoergaard <[email protected]>
Date:   Thu Oct 1 09:12:38 2020 +0200

    Update 09-positron-emission-tomography.md

commit e32a6dd
Author: mnoergaard <[email protected]>
Date:   Sun Sep 20 17:01:25 2020 +0200

    Update 09-positron-emission-tomography.md

commit 4236364
Author: mnoergaard <[email protected]>
Date:   Sun Sep 20 16:40:38 2020 +0200

    Update 09-positron-emission-tomography.md

commit e8d07bf
Author: mnoergaard <[email protected]>
Date:   Sun Sep 20 16:37:37 2020 +0200

    Update 09-positron-emission-tomography.md

commit d809894
Author: mnoergaard <[email protected]>
Date:   Mon Sep 14 15:18:36 2020 +0200

    Update 09-positron-emission-tomography.md

commit be14030
Author: mnoergaard <[email protected]>
Date:   Wed Aug 5 16:52:08 2020 +0200

    Update 09-positron-emission-tomography.md

commit 509fd90
Author: mnoergaard <[email protected]>
Date:   Wed Aug 5 15:51:17 2020 +0200

    Update 09-positron-emission-tomography.md

commit d632908
Author: mnoergaard <[email protected]>
Date:   Wed Aug 5 15:41:42 2020 +0200

    Update 09-positron-emission-tomography.md

commit bf1f279
Author: mnoergaard <[email protected]>
Date:   Wed Aug 5 15:30:24 2020 +0200

    Update 09-positron-emission-tomography.md

commit b8a644d
Author: mnoergaard <[email protected]>
Date:   Wed Aug 5 15:28:05 2020 +0200

    Update 09-positron-emission-tomography.md

commit 330826c
Author: mnoergaard <[email protected]>
Date:   Wed Aug 5 15:26:56 2020 +0200

    Update 09-positron-emission-tomography.md

commit d2ec372
Author: mnoergaard <[email protected]>
Date:   Wed Aug 5 14:01:15 2020 +0200

    Update 09-positron-emission-tomography.md

commit 8751d29
Author: mnoergaard <[email protected]>
Date:   Tue Aug 4 17:42:18 2020 +0200

    Update 09-positron-emission-tomography.md

commit 2f15845
Author: mnoergaard <[email protected]>
Date:   Tue Aug 4 17:17:52 2020 +0200

    Added blood information to 09-positron-emission-tomography.md

commit 8149212
Author: mnoergaard <[email protected]>
Date:   Tue Aug 4 16:43:31 2020 +0200

    Minor fixes in 09-positron-emission-tomography.md

commit 889a04a
Author: mnoergaard <[email protected]>
Date:   Tue Aug 4 12:49:36 2020 +0200

    Update 09-positron-emission-tomography.md

commit 10caf45
Author: mnoergaard <[email protected]>
Date:   Tue Aug 4 10:59:09 2020 +0200

    Updated filename for 09-positron-emission-tomography

commit c455805
Author: mnoergaard <[email protected]>
Date:   Tue Aug 4 10:50:52 2020 +0200

    Update PET.md to include sections on "info" and "radiochem"

    Updated tables (info and radiochem) as specified in the google doc.

commit d58e3e4
Author: Melanie Ganz-Benjaminsen <[email protected]>
Date:   Tue Aug 4 09:42:24 2020 +0200

    Edited header

commit 13c892b
Author: mnoergaard <[email protected]>
Date:   Wed Jul 22 14:01:57 2020 +0200

    Updated pet.md + added image

commit e99bb58
Author: mnoergaard <[email protected]>
Date:   Mon Jul 20 21:25:31 2020 +0200

    Update pet.md

commit 04c2d28
Author: mnoergaard <[email protected]>
Date:   Mon Jul 20 21:07:04 2020 +0200

    Added PET markdown in src
tsalo
tsalo requested changes Oct 2, 2020
View reviewed changes
Copy link
Member

@tsalo tsalo left a comment

Choose a reason for hiding this comment

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

I have done a quick pass-through looking specifically for recent developments in the specification that might have been implemented after the BEP had solidified. This isn't a full review- just a set of minor formatting/terminology changes.

In addition to my other comments, one thing that needs to be updated is the schema. This BEP doesn't seem to add any new entities, which makes things easier, but there are a number of new suffixes added under a new pet data type. Here is a schema file detailing the rules for anatomical MRI, which you can use as a template for a new datatypes/pet.yaml file.

src/04-modality-specific-files/09-positron-emission-tomography.md Outdated Show resolved Hide resolved
src/04-modality-specific-files/09-positron-emission-tomography.md Outdated Show resolved Hide resolved
src/04-modality-specific-files/09-positron-emission-tomography.md Outdated Show resolved Hide resolved
@Remi-Gau
Copy link
Collaborator

Remi-Gau commented Oct 3, 2020

@melanieganz @mnoergaard

Would it be OK if I pushed a change to make sure that every sentence starts on a new line. This would make it easier to use the Github GUI to suggest change in clear way. (See examples above)

@effigies
Copy link
Collaborator

effigies commented Oct 3, 2020

@Remi-Gau Please feel free to fix formatting.

sappelhoff
sappelhoff reviewed Apr 6, 2021
View reviewed changes
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.

@mnoergaard you added "events" in a recent commit, but I think they should also be added to the "template" that is shown here:

https://bids-specification.readthedocs.io/en/bep-009/04-modality-specific-files/09-positron-emission-tomography.html#pet-recording-data

because that's how it's done also in e.g., EEG:

https://bids-specification.readthedocs.io/en/bep-009/04-modality-specific-files/03-electroencephalography.html#eeg-recording-data

To achieve that, "events" should be added to the PET schema:

https://github.com/bids-standard/bids-specification/pull/633/files#diff-c50725de712ea522efdecd15fb0ec687ca1d96b9f40759562fa53b2ef3170988

as done here:

bids-specification/src/schema/datatypes/eeg.yaml

Lines 49 to 50 in 7f401d4

- suffixes:
- events

tagging @tsalo for review.

effigies
effigies reviewed Apr 6, 2021
View reviewed changes
src/schema/datatypes/pet.yaml Outdated Show resolved Hide resolved
src/schema/datatypes/pet.yaml Outdated Show resolved Hide resolved
src/04-modality-specific-files/09-positron-emission-tomography.md Outdated Show resolved Hide resolved
src/04-modality-specific-files/09-positron-emission-tomography.md Outdated Show resolved Hide resolved
Remi-Gau
Remi-Gau approved these changes Apr 7, 2021
View reviewed changes
Copy link
Collaborator

@Remi-Gau Remi-Gau left a comment

Choose a reason for hiding this comment

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

Forgot to mention that all my request for changes had been addressed.

@effigies
Copy link
Collaborator

effigies commented Apr 7, 2021

Is... is it time?

@melanieganz
Copy link
Contributor Author

I think so. :-)

sappelhoff
sappelhoff approved these changes Apr 7, 2021
View reviewed changes
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.

Is... is it time?

not yet! I found two blank lines 👼

but other than that, +1

src/04-modality-specific-files/09-positron-emission-tomography.md Outdated Show resolved Hide resolved
src/04-modality-specific-files/09-positron-emission-tomography.md Outdated Show resolved Hide resolved
@melanieganz
Copy link
Contributor Author

So who gets to press merge? ;-)

@effigies effigies merged commit cc46934 into master Apr 7, 2021
@effigies
Copy link
Collaborator

effigies commented Apr 7, 2021

I guess me. Happy Wednesday, everyone!

@sappelhoff
Copy link
Member

awesome :-) time to celebrate 🍾

@mnoergaard
Copy link
Collaborator

You guys are awesome! Time to celebrate! Many thanks @effigies @sappelhoff @tsalo @Remi-Gau and to all others for all the hard work!

@guiomar
Copy link
Collaborator

guiomar commented Apr 7, 2021

That's been amazing work!! CONGRATS to everyone!!

@melanieganz
Copy link
Contributor Author

melanieganz commented Apr 7, 2021 •
edited
Loading

Yes, thank you so much @effigies @sappelhoff @tsalo @Remi-Gau and of course @mnoergaard and everyone else who has put so much work in this!!! And of course @chrisgorgo thanks for getting me into this. Now on to the examples and validator finishing touches ;-)

@bendhouseart bendhouseart mentioned this pull request Jun 15, 2021
Closed
@Remi-Gau Remi-Gau deleted the bep-009 branch November 15, 2021 08:37
@mathesong mathesong mentioned this pull request Aug 15, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rwblair rwblair rwblair left review comments

@mnoergaard mnoergaard mnoergaard approved these changes

@mathesong mathesong mathesong requested changes

@KrisThielemans KrisThielemans KrisThielemans left review comments

@gllmflndn gllmflndn gllmflndn left review comments

@CPernet CPernet CPernet left review comments

@guiomar guiomar guiomar left review comments

@robertoostenveld robertoostenveld robertoostenveld left review comments

@alegiac95 alegiac95 alegiac95 left review comments

@effigies effigies effigies approved these changes

@tsalo tsalo tsalo approved these changes

@Remi-Gau Remi-Gau Remi-Gau approved these changes

@sappelhoff sappelhoff sappelhoff approved these changes

@chrisgorgo chrisgorgo Awaiting requested review from chrisgorgo

Assignees

@effigies effigies

@satra satra

@chrisgorgo chrisgorgo

@poldrack poldrack

@robertoostenveld robertoostenveld

@choldgraf choldgraf

@ChristophePhillips ChristophePhillips

@KirstieJane KirstieJane

@guiomar guiomar

@Remi-Gau Remi-Gau

Labels
BEP enhancement New feature or request PET
Projects
None yet
Milestone
1.6.0
Development

Successfully merging this pull request may close these issues.