Skip to content
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

Speeed #353

Merged
merged 23 commits into from
Jan 24, 2025
Merged

Speeed #353

merged 23 commits into from
Jan 24, 2025

Conversation

jordandekraker
Copy link
Collaborator

@jordandekraker jordandekraker commented Jan 20, 2025

Minor:

  • untar of nnunet models is run at download, and replaced with ln -s on each subsequent use
  • volumetric laplace_coords.py and create_warps.py retired

Major:

  • AP and PD coords are solved on the native midthickness surface using Laplace-Beltrami, and saved as .shape.gii files. This is much faster, and ensures that every edge vertex has one assigned AP|PD source|sink (helping make them more orthogonal)
  • instead of generating warps.nii.gz from native to unfold, native surfaces simply have their x,y coordinates replaced with AP,PD, and z coordinates set to 0. This should ensure there are no "wrinkles" during unfolding, and no interpolation is needed as in the generation and application of 3D ANTs warps.

TODO:

  • testing (I have run none yet, will likely need some bugfixing, I just didn't have time today!)
  • retire additional rules/scripts
  • wetrun for all cases to make sure we're not missing something
  • clean up dentate surfaces, they must be connected components with no holes for this code to work and i think this will be an issue still

@jordandekraker jordandekraker mentioned this pull request Jan 20, 2025
@akhanf akhanf changed the base branch from unfoldregv2 to dev-v2.0.0 January 21, 2025 20:07
@akhanf
Copy link
Member

akhanf commented Jan 22, 2025

OK if I pull out the nnunet model changes to a different PR?

@jordandekraker
Copy link
Collaborator Author

@akhanf i'm wondering if you can help me with this wetrun issue:

Error in rule warp_unfold_native_to_unfoldreg:
...
While running:
/opt/workbench/bin_linux64/../exe_linux64/wb_command -surface-apply-warpfield ./sub-03/surf/sub-03_hemi-R_space-unfold_label-hipp_midthickness.surf.gii tmp/sub-03/warps/sub-03_hemi-R_space-unfold_label-hipp_desc-greedy_from-native_to-multihist7_type-surface_xfm.nii.gz tmp/sub-03/surf/sub-03_hemi-R_space-unfoldreg_label-hipp_midthickness.surf.gii

ERROR: surface exceeds the bounding box of the warpfield

Some thoughts I had:

  • The atlas & resources/unfold_template gii files are actually trimmed to 126x254 instead of 128x256 (and also the atlas midthickness surface seems to be IO shifted by some epsilon instead of appearing exactly at 1.25mm). I don't think we have any need to trim this epsilon off all new surfaces anymore, so perhaps we should consider adjusting the atlas and resource surf.gii & refvol files accordingly. When I try this out I still can't seem to get it running though :/
  • Maybe my application of snakebids.yml's unfold_vol_ref in rewrite_vertices_to_flat.py is incorrect, but when I check the native unfolded surfaces they are in the same space as the imported atlas unfolded surfaces (except epsilon). Also the nifti images of thickness, gyrification, curvature closely match those from the atlas (i.e. no flips or translations, just the atlas is trimmed by 1 voxel). The unfoldreg seems to be working correctly.
  • Since the unfolded nifti data are widely padded, the xfm warp is also much larger than needed. When I load the surface warps and surfaces, the surface is clearly within the bounds of the warp volume (but perhaps outside of the ranges of the values in the warp, since this is an inverse and not forward warp!)
  • Are you sure that the surface unfoldreg warp file is generated correctly? Last I checked it should be made using an inverse warp, and you mentioned greedy doesn't (currently) generate an inverse warp. If you didn't run any wetruns yet you may not have noticed this..

Jordan DeKraker added 2 commits January 22, 2025 15:19
akhanf added a commit that referenced this pull request Jan 22, 2025
moved to PR #356 (branch: untar-model)
@akhanf akhanf mentioned this pull request Jan 22, 2025
@akhanf
Copy link
Member

akhanf commented Jan 22, 2025

@akhanf i'm wondering if you can help me with this wetrun issue:

sure was just about to have a look at this branch now anyways..

@akhanf
Copy link
Member

akhanf commented Jan 22, 2025

fyi, I just merged in some upstream changes/fixes (most notably the PR to revert workdir back to original location by default), this might have given you some headaches when writing work to the new temp folder each time..

was getting bounding box errors with previous approach, when the
vertices were clearly within the bounding box..
@akhanf
Copy link
Member

akhanf commented Jan 22, 2025

I actually have no idea why it is complaining about the bounding box here, as even using the wb_command file-information and surface-information the vertices are well withing the bounding box of the volume.. I've also encountered this intermittently before with some surfaces in this rule..

So I've come up with a workaround that still uses workbench to generate the surfaces - it uses the ability to get and set vertex coordinates (as metric giftis), and I just use metric math to add the warp to the coords.. Could've also just use nibabel, but this ended up being faster for me to implement..

@akhanf
Copy link
Member

akhanf commented Jan 22, 2025

The other thing I've noticed is the unfold greedy registration probably needs to be regularized much more.. we could perhaps optimize parameters (and registration algorithm, including ANTS) using experiments similar to your unfoldreg paper (ie optimize based on subfield Dice) in another PR..

@akhanf
Copy link
Member

akhanf commented Jan 22, 2025

btw the laplace beltrami outputs look great!!

also renamed laplace-beltrami.py to laplace_beltrami.py for consistency
@jordandekraker
Copy link
Collaborator Author

So I've come up with a workaround that still uses workbench to generate the surfaces - it uses the ability to get and set vertex coordinates (as metric giftis), and I just use metric math to add the warp to the coords.. Could've also just use nibabel, but this ended up being faster for me to implement..

heh this is a funny solution, but glad it works! do you think its worth posting as a wb_command issue?

@jordandekraker
Copy link
Collaborator Author

The other thing I've noticed is the unfold greedy registration probably needs to be regularized much more.. we could perhaps optimize parameters (and registration algorithm, including ANTS) using experiments similar to your unfoldreg paper (ie optimize based on subfield Dice) in another PR..

perhaps we also want to use the heavy_smooth_unfold_surf surfaces (i.e. "stingray" surfaces) in unfolded registration as well? currently they're not used, but it could be cool

@jordandekraker
Copy link
Collaborator Author

This PR is looking good to me! Perhaps we could go ahead and merge this PR and start some new ones for

  1. "stingray" and/or regularized unfoldreg
  2. fixing DG native surf and AP/PD coords

Copy link
Member

@akhanf akhanf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

couple comments - I forgot to hit enter on these earlier --

@akhanf akhanf marked this pull request as ready for review January 24, 2025 14:29
@akhanf akhanf merged commit 552420f into dev-v2.0.0 Jan 24, 2025
4 checks passed
@akhanf akhanf deleted the speeed branch January 24, 2025 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants