-
Notifications
You must be signed in to change notification settings - Fork 14
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
bugfix for warps hemi-L #180
Conversation
But does this then mean you would have manually flip before applying the warp? |
nope, no manual flip required. the warp is now created with the coords already flipped back to their original orientation. thus, we don't need to flip again manually and we don't need to (as we previously did) have a step that concatenated a flipping transform |
Some rules in gifti.smk were still causing warps with Overall this PR cleans up the code a fair bit too, removing a few rules. |
ok just about to try running the new updates -- but wanted to first just see what bug you are referring to. I transformed data from native to unfold and back again, both with left and right, and didn't come across any issues (aside from funny things at the boundary likely related to inverse inconsistency) -- what issues were you having?
|
@@ -251,7 +251,7 @@ rule create_warps_dentate: | |||
dir="AP", | |||
label="dentate", | |||
suffix="coords.nii.gz", | |||
desc="init", | |||
desc="laplace", |
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.
doesn't this change the behaviour to now use laplace instead of shape-injection initialized fields for the dentate?
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.
Currently, they're the same thing actually (see rule linked below)
However, if we do end up refining the Laplace fields in the dentate in the future, then this is the filename we will want to use.
rule laplace_coords_dentate: |
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.
ah ok, I'm guessing we added that cp rule to avoid changing later rules.. I would prefer actually removing that cp rule to avoid confusion, but can perhaps can clean that up in another PR..
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.
we actually still use it to populate the coords folder in the output directory as well
I'm actually wondering if my issue was specific to v1.0.0. I don't see any changes that seem relevant since then, but maybe I missed some. The issue is that in hemi-L, if you unfold and then refold (or unfold the right hemisphere and refold it to the left hemisphere space), the image is the right orientation and so it loads as a subfield overlay in ITKSNAP, but the header affine still places it on the opposite side of the brain. I came across the issue while trying to set up 2D registrations in unfolded space, and it will become a bigger issue if we want to concatenate unfold-2dregister-refold transforms all together. |
yes, the workflow was considering unfolded L and Lflip to be distinct, so I guess the main change here is just to use the same unfolded space for L and R (ie not have a Lflip space). That certainly does simplify things, I would just want to make sure we aren't breaking anything along the way. I can't recall why we had the distinction between Lflip and L in the unfolded space in the first place.. Only thing I can think of is if we wanted to allow for different physical unfolded coordinates for left and right (e.g. side-by-side) - they are identical right now, which is why I think it just works to remove the L and Lflip distinction, but it would mean we couldn't do that in the future I guess? |
By all means, I was hoping you'd help me make sure we're not breaking something with this change. By my understanding, the actual unfolded space is always the same even before we eliminate hemi-Lflip. The thing that mainly changes is that create_warps.py now finds an interpolation from unfolded to xyz in hemi-L instead of unfolded to xyz in hemi-Lflip. As far as I know, nnunet.smk, shape_inject.smk, and the Laplace initialization for autotop.smk (that comes from shape_inject.smk) must be done in hemi-Lflip. Laplace coords then need to be flipped back to hemi-L to populate the output coords folder, and once that is done I don't think there are any other steps that need to be in hemi-Lflip |
Yep I don't see a major downside to eliminating Lflip in unfolded space, since we aren't setting L and R to be different there, and it does simplify things alot! I'll run it through end-to-end and make sure the surfs and vols get warped properly. |
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.
tested end to end transforming vols and surfs and all looks good!
nice catch to eliminate extra complexity! I'll merge this in
I noticed there were still some issues with the warps
from-unfold_to-native
. I think this can be solved and the code simplified by simply running create_warps AFTER unflipping.For the record, i think there is a difference between concatenating
desc-flipLR_type-itk_xfm.txt
onto existing transforms and flipping withc3d -flip x
. I think one of these applies to the header only, while the other applied to the actual image. These should be equivalent, but i don't think that ANTs reads them the same way. Either way, this circumvents the issue entirely.I tested once for a standard T1w modality and once for a cropseg modality (where output space is corobl) and both seem to work.