-
Notifications
You must be signed in to change notification settings - Fork 47
ENH: Add SpatialReference field to some outputs #513
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
base: master
Are you sure you want to change the base?
Conversation
|
For TemplateFlow volumetric spaces, the SpatialReference field will just have a URL (e.g., |
|
Does it make more sense to copy the BIDSURI functionality from fMRIPrep? I think threading a |
|
That makes sense. I can add that. |
|
I think it looks good now. Here's the value from ds054: {
"SpatialReference": "bids:templateflow:tpl-MNI152Lin/tpl-MNI152Lin_res-02_T1w.nii.gz"
} |
effigies
left a comment
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.
A few thoughts. I'm not sure that they're enough to finish this off on, but LMK what you think, and we can talk this over next week maybe?
| spatial_reference = pe.Node( | ||
| TemplateFlowReference(), | ||
| name='spatial_reference', | ||
| ) |
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.
I'm worried this is going to interfere with passing dataset_links from fmriprep, since that maps TF_LAYOUT.root onto bids:templateflow: and accepts raw paths.
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.
I think we should update fMRIPrep to point to the website when it's a built-in template, but I could pass the dataset_links into this node to use a local path if templateflow is a key in the dictionary and doesn't start with http.
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.
One reason I was leaning toward the website is that the CIFTI SpatialReference dictionary uses the website. If we do commit to using the local templateflow location then we should change that as well.
| if template_name in tf.TF_LAYOUT.get_templates(): | ||
| self._results['uri'] = f'{tf_url}/{str(rel_path)}' |
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.
Presumably this is going to return custom templates found in TF_LAYOUT, so I'm not sure that the else branch will ever get hit.
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.
Oh... I had hoped that get_templates would only return built-in templates, but that makes sense. Is there any way to distinguish built-in templates from custom ones?
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.
Hmm. I suppose we could inspect the skeleton in the templateflow client.
import zipfile
import templateflow
templates = [
tpl.name.removeprefix('tpl-')
for tpl in zipfile.Path(templateflow.conf.load_data('templateflow-skel.zip')).iterdir()
]That's very much unsupported API, but it's doable.
Co-authored-by: Chris Markiewicz <effigies@gmail.com>
Related to nipreps/fmriprep#3579.
This should add SpatialReference to volumetric, standard-space anatomical outputs.