-
Notifications
You must be signed in to change notification settings - Fork 120
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
Voxel size passing and hires pipeline selection changes to allow consistent processing #225
Conversation
I still have to test the changes. |
help="Choose the primary voxelsize to process, must be either '1' or 'auto'. '1' forces " | ||
"processing at 1mm voxel size, 'auto' enables processing at higher resolutions (conforming" | ||
" to the smallest voxel edge length). Select 'auto' (default) for hires processing."), | ||
'--vox_size', type=__vox_size, default="min", dest='vox_size', |
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.
Should this not include a disclaimer that if the res is above 0.95 it will be set to 1.0 mm with min?
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.
Well, all that conforming did not actually happen yet :(
So I think I added that now. I'll have to test the code now.
The changes to recon-surf.sh seem overly complex to me. a.) hires_voxsize_threshold=0.999 is fixed in the beginning. This can not be changed via the interface. Why not have a vox-size threshold default argument in the function call to conform.py --check-only? This way this does not need to be present in recon-surf. b.) Why have the exact vox-size written to a separate file? Why not just a boolean value (or sth. like "Conform-min", "1"). And then check for just that one value + set conform-min/hires flag accordingly? If this is about making sure the resolutions between segmentation and orig are equal: |
Unfortunately, there are some nasty dependencies in the code that make many easy solutions non-viable. There are 4 interfaces that all need to have a sensible behavior.
|
f089dff
to
079c46e
Compare
I cleaned up the recon-surf file as well. I usually code on the side of clear call interfaces and good error handling, that kind of goes out the window here, but should probably still work with the changed (shorter) structure -- and different interface in conform.py |
079c46e
to
dbd2960
Compare
…istent processing. Interface change to conform.py - Adds new flag: --conform-size, analogeous to mri_convert --conform_size Interface change to recon-surf.sh, run_prediction.py and run_fastsurfer.sh. - Remove --vox_size from recon-surf.sh, vox-size is automatically extracted from the image file - Add additional options to --vox_size in run_prediction.py and run_fastsurfer.sh: Now accepts a voxel size between 0.7 and 1 (which forces that specific voxel size) or 'min' (default), which performs conforming to the minimal voxel size (akin to mri_convert --conform_min). Note: the naming 'auto' has been removed in favor of 'min'. Lower than 0.7 prints a experimental warning. Higher than 1 exists with an error. The function in conform.py have been adapted/extended to allow the passing of the 'target voxel size'. The conform_min argument was therefore superceeded by a conform_vox_size argument, which can also be 'min' to get the conform_min behavior. FastSurferCNN/data_loader/conform.py - fix the assignment of the fov field in the header - move funtions FastSurferCNN/data_loader/data_utils.py - update the changed call interface to conform functions new FastSurferCNN/utils/arg_types.py - added type functions to check the arguments vox size, target dtype, and conforming to 1mm options. FastSurferCNN/utils/parser_defaults.py - add options to set the threshold at which we conform to 1mm - update help texts - use type functions to arg_types FastSurfer/run_prediction.py - add functionality to conform to 1mm - change vox_size auto option to min - update call to conform functions - re-enable CUDA out of memory error handling recon_surf/recon-surf.sh - integrate the threshold for vox size for hires processing - adapt conform calls to use updated interface - remove the --vox_size and --hires options from the interface run_fastsurfer.sh - change vox_size auto option to min - update the help text for --vox_size - update the validity checks and error messages for --vox_size - update the parameters to recon-surf
dbd2960
to
c8d8be2
Compare
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.
recon-surf.sh can be simplified even further in my opinion. I do not like the conform_to_1_threshold name because it implies you are conforming to (any) one resolution and not to 1.0 mm.
clean up if statements in recon-surf
Can also be added later, but right now, we cannot change the --conform_to_1mm_threshold in |
Interface change to conform.py
Interface change to recon-surf.sh, run_prediction.py and run_fastsurfer.sh.
Now accepts a voxel size between 0.7 and 1 (which forces that specific voxel size)
or 'min' (default), which performs conforming to the minimal voxel size (akin to mri_convert --conform_min).
Note: the naming 'auto' has been removed in favor of 'min'.
Lower than 0.7 prints a experimental warning.
Higher than 1 exists with an error.
The function in conform.py have been adapted/extended to allow the passing of the 'target voxel size'.
The conform_min argument was therefore superceeded by a conform_vox_size argument, which can also be 'min'
to get the conform_min behavior.