[ENH]: Add space awareness to existing components #268
No reviewers
Labels
No labels
CRITICAL
Stale
WIP
bug
concept
coordinate
dataset
dependencies
documentation
duplicate
enhancement
github_actions
good first issue
help wanted
invalid
maintenance
maps
marker
mask
on hold
parcellation
preprocess
question
ready
storage
template-space
triage
wontfix
No milestone
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
juaml/junifer!268
Loading…
Reference in a new issue
No description provided.
Delete branch "enh/space-awareness"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
This PR adds space awareness to existing components, enabling easy addition of spaces and transformation between them.
Codecov Report
100.00% <ø> (ø)90.37% <53.21%> (-1.16%)Flags with carried forward coverage won't be shown. Click here to find out more.
100.00% <100.00%> (ø)97.72% <100.00%> (+0.10%)97.56% <100.00%> (+0.12%)98.36% <100.00%> (ø)97.93% <ø> (ø)100.00% <ø> (ø)94.11% <ø> (ø)100.00% <100.00%> (ø)100.00% <100.00%> (ø)83.33% <50.00%> (-13.10%)Does it make sense to tackle #255 on this one to? Basically instead of using
MNIuse the right MNI and then raise error if the space does not match. Otherwise this will be corrected in the next days, giving an somehow unstable main branch.Yes totally. That's my plan as well. Will spend some time to get it correct and then we can add native space with a couple of commits.
perfect, will wait to review then.
Should be good to review now.
Ok my bad, I corrected some stuff which broke the tests, will fix it.
Good for review now.
@ -279,6 +282,7 @@ Available| ``TianxS2x3TxMNInonlinear2009cAsym``,| ``TianxS3x3TxMNInonlinear2009cAsym``,| ``TianxS4x3TxMNInonlinear2009cAsym``We need to remove the space from the name.
@ -300,3 +305,4 @@- ``MNI152NLin2009cAsym``- 0.0.3- | Shen, X., Tokoglu, F., Papademetris, X., Constable, R.T.| Groupwise whole-brain parcellation from resting-state fMRI dataSince we add the
spaceparameter, we need to remove the space from the name.How sure are we that is the Linear 6th Gen Asymmetric?
@ -214,3 +252,4 @@all_spaces = []for t_mask in true_masks:if isinstance(t_mask, dict):mask_name = next(iter(t_mask.keys()))We can't intersect masks in different MNI spaces.
@ -42,7 +45,7 @@ if TYPE_CHECKING:# TODO: have separate dictionary for built-inName should be SUIT
@ -100,4 +107,4 @@}elif year == 2019:_available_parcellations["Shen_2019_368"] = {"family": "Shen",Same here for the names, it should not contain the space now that we have a dedicated field.
@ -236,17 +258,89 @@ def get_parcellation()we only merge when it's the same space.
@ -121,6 +121,10 @@ class DataladAOMICID1000(PatternDataladDataGrabber):out = super().get_item(subject=subject)We need to find the right MNI space version.
@ -166,8 +166,12 @@ class DataladAOMICPIOP1(PatternDataladDataGrabber):out = super().get_item(subject=subject, task=new_task)get right space name
@ -166,6 +166,10 @@ class DataladAOMICPIOP2(PatternDataladDataGrabber):out = super().get_item(subject=subject, task=f"{task}_acq-seq")same here
@ -55,7 +55,10 @@ class OasisVBMTestingDataGrabber(BaseDataGrabber):"""I'd rather use the right space here, so we test correctly the masks/parecellations.
@ -141,8 +144,8 @@ class SPMAuditoryTestingDataGrabber(BaseDataGrabber):anat_fname = self.datadir / f"{subject}_T1w.nii.gz"I'd rather use the right space here, so we test correctly the masks/parecellations.
@ -236,12 +239,13 @@ class PartlyCloudyTestingDataGrabber(BaseDataGrabber):"""I'd rather use the right space here, so we test correctly the masks/parecellations.
Do we need "space" for the confounds file? It does not make any sense to me.
The mask was made with CAT12 which uses SPM12 and that uses its own template which is similar to MNI Linear 6th Generation Asymmetric, at least that's what I found out given the not-so-proper documentation.
We need to update the tests so the data is not in "MNI" space but in "MNINLin6..." or something specific.
Can you check with @kaurao and Sam? They might have change the template. Mainly what I'm doubting is the Linear/NLinear difference.
@ -214,3 +252,4 @@all_spaces = []for t_mask in true_masks:if isinstance(t_mask, dict):mask_name = next(iter(t_mask.keys()))I think at this point we should not have the "inherit" as it was already computed before. Usually the "inherit" is a callable.
This will have conflicts with other non-MNI spaces. I think we need to check for either "native" or not any standard space supported by junifer.
@ -42,7 +45,7 @@ if TYPE_CHECKING:# TODO: have separate dictionary for built-inTo be done in another PR
@ -61,27 +65,28 @@ for scale in range(1, 5):"family": "Tian",We need confirmation that this is the exact space.
@ -100,4 +107,4 @@}elif year == 2019:_available_parcellations["Shen_2019_368"] = {"family": "Shen",To be done in another PR
@ -236,17 +258,89 @@ def get_parcellation()Same as with masks. MNI is only one of the spaces we support. We also have SUIT. This will fail for non-native and non-MNI space.
@ -804,7 +907,11 @@ def _retrieve_suit(resolution = closest_resolution(resolution, _valid_resolutions)I would leave the right space and not just MNI in the filename. Even if this triggers a second download on old users.
I wanted to get the native check in the native support PR. About standard spaces supported by junifer, there's not really a list of them now but in that case we need to make one, would you agree to making one?
@ -61,27 +65,28 @@ for scale in range(1, 5):"family": "Tian",To be exactly specific, this is what SPM 12 uses:
IXI549Space(ref). Now, on the MNI side, it's close toMNI152Lin6Asym(ref). So, I can change it toIXI549Spaceif you agree.@ -236,17 +258,89 @@ def get_parcellation()I wanted to get multi-space support in a different PR as we discussed.
I checked and it's based on the TPM and Shooting GM masks from CAT12 which uses SPM12. SPM12 uses
IXI549Spaceas its space and on the MNI side it's closest toMNI152Lin6Asym. So, like my other comment, I can change it toIXI549Space.For me it does not make much sense to have a "list" of supported at this moment. A user can create a dataset with space "XYZ" and then add a mask/parcellation for such space and it should work.
Here the condition to test is "native" and not if it's MNI. As the idea is to warp from whatever the mask is to "native" space. Now this "whatever" needs to match the available transform space.
@ -61,27 +65,28 @@ for scale in range(1, 5):"family": "Tian",Is not what I meant. They provide the atlas for SPM12, and that table shows the default.
From the paper:
Pre-processing was performed using Statistical Parametric Mapping subroutines (SPM5)
Later on:
normalized to the stereotaxic space of the Montreal Neurological Institute (MNI) template ([Ashburner and Friston, 2005]
But they don't specify which MNI space.
From the README of v2:
But again, we don't know much about the v1 or the "guess".
@ -61,27 +65,28 @@ for scale in range(1, 5):"family": "Tian",When you check https://www.gin.cnrs.fr/en/tools/aicha/ you would see they offer both v1 and v2 for SPM12 which we use. Now after that, it goes back to the reply I had earlier.
@ -61,27 +65,28 @@ for scale in range(1, 5):"family": "Tian",New take:
IXI549SpaceWhat do you think?
@ -61,27 +65,28 @@ for scale in range(1, 5):"family": "Tian",Sounds good.