-
Notifications
You must be signed in to change notification settings - Fork 169
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
James Kent edits to bep038 (Atlas Specification) #1764
base: bep038
Are you sure you want to change the base?
Conversation
Thinking about adding an Case 1 (sharing an atlas)
Case 2 (using a group level atlas resampled into individual spaces
case 3 (a subject-specific derived atlas, the same as the original)N/A the current JSON file has information that assumes the atlas is being expressed in a single space and that is primarily it. The other one should be the
Okay, so if the general use cases can be broken down into 3 buckets:
Let's test out the 3 use cases Case 1: sharing an atlas as a standalonebasic
The atlas is only projected onto one template space multiple spaces
You could just have one not have the space label, but that wouldn't be recommended. multiple spaces (valid, but not recommended)
Use-case 2: resampling an existing atlas into subject space/resolution/etcbasic
The json file at the subject level overwrites the top level json file, so the correct spatialReference and resolution is found. Use-case 3: subject-specific atlases
The atlas_description.json could be added at the top level if the method of generation for each subject was the same. |
This looks good to me. Though I think we will want a |
for more information, see https://pre-commit.ci
- probseg | ||
- mask | ||
extensions: | ||
- .label.gii |
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.
- .label.gii | |
- .gii | |
- .label.gii |
To account for scalar-based atlases
For commit 14098b3 strengths
drawbacks
|
Dear @jdkent, thank you so much for all the work you have put in to consolidate both standpoints with fresh eyes! 🙏 The OpenneuroPET team which was also one of the drivers behind the atlas BEP highly appreciates that. |
Hi @melanieganz , I've got another round of edits to make to fully encompass the feedback I got during the meeting, so please hold off trying to understand the mess 😆, wrt your comment:
Talking with @rwblair, it sounds like this is a relatively high-priority item to make a derivative dataset uploadable to openneuro. |
Hi James, The issue can see is that those changes do not allow what was also intended, ie to share an atlas on it's own (or I missed something? - all the examples are /derivatives - re Case 1 ) To give everyone the context, there was a discussion about 1.5 years ago about where 'atlas' should be shared - being as usage or for sharing on it's own. It was 'decided' that sharing at the root level was best because: Back to the PR, I'd suggest allowing the folder PS: can't share on it's own because if no sub- in root, then not valid |
That is a good concern, definitely want to keep the ability to share an atlas on its own. From my understanding of the derivative common principles (see storage of derived datasets, point number 2), The will look into that. |
Very good point @jdkent we could show both options for case 1, adding in the the dataset_description.json DatasetType. What do you think?
-- and yes you are right it has consequences on the validation - I will make an example which indeed will remind us to 'promote' @melanieganz @effigies is there a BIDS-example repo fork we can push stuff for this PR? |
If one does not exist, I would suggest just making a bep038 branch on the main repo. |
I just made a branch of the bids-examples according to what @effigies suggested. So we can add stuff there: https://github.com/bids-standard/bids-examples/tree/bep038 |
Dear @jdkent, @CPernet, @effigies, @pwighton and @oesteban, as suggested by @effigies I added a bep038 branch to the bids examples and following the example structure that Cyril suggested above I added two examples for atlas001 (raw dataset) and atlas002 (derivative dataset) that are very generic and that we can base our discussion on. I also added an atlas003 example where I tried to base it on the PET atlas that @pwighton has already shared. Can you please look at them and let me know if that makes sense and if that covers your needs or what changes are necessary? Also please note, when making the examples in the bids-example repo I discussed the example Cyril had made above with him and he corrected some misspelling there through me. |
Hello everyone, thank you very much for your continued discussion and work on this. We also started Thanks again. Best, Peer |
Dear @jdkent , we really, really would like to have this move forward. But you anted to fix the examples so they align with your ideas. Can we help with this? Can you point me to the right examples in the text and then I can make examples for the validator to check? |
apologies, I am setting some time this week to finish this up. |
Hi @melanieganz, I have updated the examples here |
Hi @jdkent, thank you! So just that we agree, I can take the examples from the .md you linked to and add them to/modify the existing examples in https://github.com/bids-standard/bids-examples/tree/bep038 and then we are good to check this with @rwblair? |
yes, I am in agreement with modifying those examples and then checking in. |
@melanieganz I am working on uploading PS13 to templateflow, showing how that tool would encode it. I hit an issue with these examples atlas003/atlas/petsurfer/atlas-ps13_hemi-{R,L}_space-fsaverage_stat-mean_desc-nopvc_mimap.nii.gz NIfTI files are borderline "valid" to allow hemi- (just because we haven't explicitly forbidden it, but it feels like we should). These should be GIFTIs as they encode surface information. Other than that, I cannot provide more useful feedback because I don't think the proposal is in good shape. I've been preparing a PR for the last three months and hope to finish it today -- apologies for the delays. |
Rendered proposal: https://bids-specification--1764.org.readthedocs.build/en/1764/atlas.html
random thoughts for notekeeping
looking at Oscar's suggestions in this comment for changes in the file structure:
#1281 (comment)
the file structure changes look like an improvement to me (nothing appears to drastically change)
CASE 1:
original
Oscar's proposal
What I like about Oscar's structure is that:
For findability/searchability there is debate as to whether an atlas or a template should be a first class citizen. Looking at the templateflow website, it is not clear what atlases are available. Adding the tsv/json at the top level will make that much more clear, (and ideally, each atlas could be expressed in each template space, user beware!).
BIDS maintainer proposal (making an atlas as its own derivative dataset expressed in one or more spaces)
This is a concept mapping from tpl being a reference and space being an application of the reference, so the tpl directory becomes an atlas. The order of entities is a little weird, since the folder starts with space, but then atlas becomes a first class citizen again. This use case is for packaging an atlas by itself, the primary use-cases deriving this
BIDS maintainer proposal (making an atlas as its own derivative in one space)
where the spatial reference is defined in the .json file and the space directory is redundant.
OR
do the same thing as the previous example and include the
space-<spacelabel>
directoryCase 2
original
Oscar's proposal
I also prefer Oscar's suggestion here because:
There is the magic of how the atlases got into a particular space, but the
spatialReference
defined in the top level .json
file should identify the original template used by the atlas. For practicality of sharing, I would probably also include the actual atlas image at the top level of derivatives so that people can more easily apply the necessary transformationsJames' proposal
BIDS maintainer proposal (space
If someone has multiple atlases in a dataset, it may get crowded in the top level with the number of files. so placing the files in subdirectories help with the aesthetics.
Case 3:
Original
Oscar's proposal:
No change
This one also looks fine to me!
Overall I find Oscar's suggestions clean, and I'm fine with treating an atlas as a modular derivative (that does not need to reside in a BIDS-raw dataset).
BIDS Maintainer's proposal
Also no changes for this use case.
Case 4 (just referencing an external atlas)
/derivatives/
dataset_description.json # {"SourceDatasets": [ {"URL": "../../atlas-/"} ]
sub-01/
func/
sub-01_atlas-_[dseg|probseg|mask].[nii|dscalar.nii|dlabel.nii|label.gii|tsv][.gz]
sub-01_atlas-space-[dseg|probseg|mask].[nii|dscalar.nii|dlabel.nii|label.gii|tsv][.gz]
I come from a very MRI-centric perspective, and from that perspective I think Oscar's proposed changes do not impede those use-cases (AFAICT).
should an atlas be its own type of dataset (e.g., raw, derivatives, atlas)?
What does making an atlas different than a
derivative
buy us? Does it make validation easier?Does it restrict what can go in that dataset? Does it make sharing easier?
Maybe I want to include an atlas with multiple other derivatives in my pipeline output, do I need to separate out
the atlas files to make my pipeline output sharable?