[ENH]: Update Warp datatype schema and logic #390
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!390
Loading…
Reference in a new issue
No description provided.
Delete branch "refactor/warp-dtype"
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?
Warpdatatype to be a "list" of mappings from / to spaces."warper"key to theWarpdatatype indicating which warper was used:fslorantsfor the moment.SpaceWarperto allow to"auto"select the warper based on the dataset. This should require both FSL and ANTs as external dependencies. This only works if reference is"T1w":"auto"is not defined for junifer internal transforms (i.e. between template spaces)Codecov Report
All modified and coverable lines are covered by tests ✅
Additional details and impacted files
100.00% <ø> (ø)Flags with carried forward coverage won't be shown. Click here to find out more.
50.00% <ø> (ø)86.20% <ø> (+5.56%)50.00% <ø> (ø)29.62% <ø> (-1.41%)50.00% <ø> (ø)83.33% <ø> (+2.95%)77.77% <ø> (+1.91%)50.00% <ø> (ø)89.53% <ø> (+0.87%)100.00% <ø> (ø)I did not repeat my comment several times, but the logic to find the right warper/transform needs to consider both the src and dst spaces everywhere.
Maybe a util function can sever as such, also checking if more than one transform is available and raising an error in such cases.
The only special case is coordinates, as long as one MNI space is available, it should work.
can we define a type to avoid this mess here?
@ -146,14 +146,23 @@ Warping to subject's native spaceTo warp to subject's native space, the dataset needs to provide ``T1w`` andThere's no "preferred", but the one that suits the datagrabber's warp file format.
@ -37,10 +37,8 @@ class ANTsCoordinatesWarper:target_data : dictThe corresponding item of the data object to which the coordinateswill be applied.Don't we also need to check that the "warper" is "ants"?
This logic needs to be redefined.
We could have fMRIPrep output in which we have both MNI6thGen and 2009c to T1w... which one are we choosing?
@ -37,10 +37,8 @@ class FSLCoordinatesWarper:target_data : dictThe corresponding item of the data object to which the coordinateswill be applied.Same as before.
@ -83,1 +86,3 @@if dst == "T1w":if dst == "native":# Warp data checkif warp_data is None:Same as before
why is this T1w here?
@ -203,0 +200,4 @@if not isinstance(depends_on, list):depends_on = [depends_on]for entry in depends_on:# Check dependencies@ -36,17 +36,21 @@ class UpdateMetaMixin:for k, v in vars(self).items():same here, if it's not a list, make it a single element list and avoid duplicated code.
That's a fair point. What about having a
junifer.typingmodule to take care of this and a lot of other type hinting mess across the codebase?+1
Resolving this as it's now being tracked in #392 .
@ -83,1 +86,3 @@if dst == "T1w":if dst == "native":# Warp data checkif warp_data is None:Because that's provided here:
github.com/juaml/junifer@2d974e1f1c/junifer/data/masks/_masks.py (L536)@fraimondo Updated the logic now, would appreciate your feedback.
The logic is now good.
There are some comments I made on some issues that are not related to this refactor, but might be worth addressing here.
Like assuming that
dst == "T1w"means warp to native space.@ -83,1 +86,3 @@if dst == "T1w":if dst == "native":# Warp data checkif warp_data is None:I meant that it does not make any sense to asume that T1w is native space.
The comment says "native space warping" and then check if
dst = "T1w". This would be true if the space of the T1w is native.@ -69,3 +83,4 @@# Create element-specific tempdir for storing post-warping assetselement_tempdir = WorkDirManager().get_element_tempdir(prefix="fsl_warper"Based on this logic and the
get_native_warperfunction I asume that it's not possible for a datagrabber to provide transforms for the same space pair using two different warpers. Is this assumption correct?If so, can we add the check in the "validate" section of the datagrabber to make it more explicit?
@ -69,3 +83,4 @@# Create element-specific tempdir for storing post-warping assetselement_tempdir = WorkDirManager().get_element_tempdir(prefix="fsl_warper"Yes that's why we raise an error now just as we discussed. We can update the logic to take care of that when we actually have the transform files for two different warpers.
@ -69,3 +83,4 @@# Create element-specific tempdir for storing post-warping assetselement_tempdir = WorkDirManager().get_element_tempdir(prefix="fsl_warper"Not sure I get you, can you please explain?
@ -83,1 +86,3 @@if dst == "T1w":if dst == "native":# Warp data checkif warp_data is None:Fair enough.
Addressed.
@ -69,3 +83,4 @@# Create element-specific tempdir for storing post-warping assetselement_tempdir = WorkDirManager().get_element_tempdir(prefix="fsl_warper"I got mixed up. We don't really have a "validator" for post-datagrabbing consistency.
What I wanted to prevent is that someone creates a datagrabber which gives multiple warping options... not for now.
@ -69,3 +83,4 @@# Create element-specific tempdir for storing post-warping assetselement_tempdir = WorkDirManager().get_element_tempdir(prefix="fsl_warper"Resolving then.