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

Add T2w dorsal rootlets labels #158

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

valosekj
Copy link
Contributor

This PR adds dorsal C2-C8 rootlets labels for T2w images of 24 subjects. The labels were part of the "D5 dataset" used for training and testing of the "M5 model":

  • 21 images were used for the rootlets model training
  • 3 images were used for the evaluation of the inter-rater variability and rootlets model testing. Thus, 5 labels (4 manual raters + STAPLE ground truth, details here) are uploaded for each of these 3 images.

Based on this discussion, I chose the following suffixes according our intranet:

  • sub-amu01_T2w_label-rootlets_dseg.nii.gz - training label (one label per image)
  • sub-amu02_T2w_label-rootlets_dseg_desc-rater1.nii.gz - testing label (5 labels per image --> using desc- to distinguish between raters)

The rootlets labels were created from the raw T2w images (i.e., no reorientation or resampling was done) --> I included "SpatialReference": "orig" into JSON sidecars.

@mguaypaq, could you please check the PR and merge? Thanks!

@valosekj
Copy link
Contributor Author

valosekj commented Feb 16, 2024

Hold on with merging.

According to this comment, desc-rater should NOT go at the end as currently done in this PR:

$ tree derivatives/labels/sub-amu02/anat/ 
...
├── sub-amu02_T2w_label-rootlets_dseg_desc-rater1.json
├── sub-amu02_T2w_label-rootlets_dseg_desc-rater1.nii.gz
├── sub-amu02_T2w_label-rootlets_dseg_desc-rater2.json
├── sub-amu02_T2w_label-rootlets_dseg_desc-rater2.nii.gz
├── sub-amu02_T2w_label-rootlets_dseg_desc-rater3.json
├── sub-amu02_T2w_label-rootlets_dseg_desc-rater3.nii.gz
├── sub-amu02_T2w_label-rootlets_dseg_desc-rater4.json
├── sub-amu02_T2w_label-rootlets_dseg_desc-rater4.nii.gz
├── sub-amu02_T2w_label-rootlets_dseg_desc-staple.json
├── sub-amu02_T2w_label-rootlets_dseg_desc-staple.nii.gz
...

@jcohenadad
Copy link
Member

it would be good if @NathanMolinier can vet this (sorry Nathan, you are now our internal BIDS consultant 😅)

@NathanMolinier
Copy link
Contributor

Hold on with merging.

According to this comment, desc-rater should NOT go at the end as currently done in this PR:

Yes indeed, doing this will help us all when trying to identify data for a given dataset. I did not add any specific rules in our intranet to follow a specific order when using entities, but the least we should do is to append the entity label-XXX at the end right before the suffix (which is the last thing we should have before the extension).

@valosekj
Copy link
Contributor Author

valosekj commented Feb 16, 2024

but the least we should do is to append the entity label-XXX at the end right before the suffix (which is the last thing we should have before the extension).

Okay! Thanks for the confirmation; I will update the filenames in this branch.

Just a side note:

I did not add any specific rules in our intranet to follow a specific order when using entities

I wrote this function fetching individual BIDS entities using regular expressions based on their keys (e.g., sub-, ses-). The function can be extended to any other entities, and its advantage is that it is "order-agnostic" (thanks to regular expressions). Maybe it might also be useful to you.

@NathanMolinier
Copy link
Contributor

I wrote this function fetching individual BIDS entities using regular expressions based on their keys (e.g., sub-, ses-). The function can be extended to any other entities, and its advantage is that it is "order-agnostic" (thanks to regular expressions). Maybe it might also be useful to you.

Great ! That's how BIDS should be used indeed ! Entities are using keys to allow automation ! That's why, for me, BIDS should not use suffixes at all, they're basically just random values not attached to any keys.

@valosekj
Copy link
Contributor Author

I updated the filenames to NOT go with desc-rater at the end in cb54bc7.

For example:

sub-amu02_T2w_label-rootlets_dseg_desc-rater1.nii.gz --> sub-amu02_T2w_desc-rater1_label-rootlets_dseg.nii.gz

@NathanMolinier, could you please confirm, that the rootlets labels are now okay?

@NathanMolinier
Copy link
Contributor

Hey @valosekj ! This new naming version is indeed better ! Also, I would like to add a small comment: I see that you are using the suffix _dseg because I imagine you are using discrete segmentations (label values may go higher than 1: not binary). However, ideally, the mapping between label values and the segmented structure should be referenced somewhere (on our intranet like discs and vertebrae or directly within the dataset). So, is this mapping available somewhere ? Apart from that the filenames are good to go !

@valosekj
Copy link
Contributor Author

valosekj commented Mar 27, 2024

Hey @valosekj ! This new naming version is indeed better !

Great, thanks for taking the look!

Also, I would like to add a small comment: I see that you are using the suffix _dseg because I imagine you are using discrete segmentations (label values may go higher than 1: not binary).

Exactly! Rootlet labels have values from 2 to 8.

However, ideally, the mapping between label values and the segmented structure should be referenced somewhere (on our intranet like discs and vertebrae or directly within the dataset). So, is this mapping available somewhere ? Apart from that the filenames are good to go !

Good point! Currently, the mapping is avaialble, for example, in the GIF in the rootlets README. But yes, adding a section/tutorial about rootlets to https://spinalcordtoolbox.com is a good idea. I've put it on my to-do list (ivadomed/model-spinal-rootlets#39).

@mguaypaq, can you please merge?

Copy link
Member

@mguaypaq mguaypaq left a comment

Choose a reason for hiding this comment

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

Looks fine from my end, in terms of git annex get.

@mguaypaq
Copy link
Member

Ah, the dataset validator is failing because it's too old and doesn't run properly, not because it identified a real issue. I'll have to fix it before merging this PR.

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.

4 participants