-
Notifications
You must be signed in to change notification settings - Fork 16
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
Replace registration to use ANTsPy instead of Nipype #90
Conversation
@oesteban : I don't see circleci running the tests on this PR. Do we need to change the circleci config somehow to run on PRs? |
terminal_output="file", | ||
from_file=pkg_fn( | ||
"eddymotion", | ||
f"config/dwi-to-{reg_target_type}_level{i_iter}.json", |
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.
One question to mull over is how do we pass this config to ants.registration
? Right now, it's no longer being passed, and it uses the default settings of that function instead.
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.
As per discussion over email, we offline run registration.cmdline
and the massage the output CLI to replace image paths with ants images.
), | ||
fixed_image=str(fixed.absolute()), | ||
moving_image=str(moving.absolute()), | ||
**align_kwargs, |
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.
Maybe we can use the align_kwargs
variable to pass the default values in.
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.
I'm leaning towards just storing one YAML file with the command lines that the estimator can pick.
predicted, | ||
dwdata.affine, | ||
fixed, | ||
clip=reg_target_type == "dwi", |
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.
Looks like the clipping depends on the type of the fixed image (whether it's a b=0 or a diffusion-weighted volume). I guess we need to reincorporate clipping somehow.
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.
Yes, it's great you guys are bringing this up to the forefront.
Basically, the question here is that I have come to the conclusion that, even though ANTs has histogram equalization, it really needs help from the outside.
In general, there are a bunch of problems/situations where ants doesn't work very well:
- Presence of negative values (unless they are meaningufuly correct - e.g., if you are registering gradients)
- Presence of long tails (blood vessels on T1w images typically add very bright, but few, pixels which extend the histogram)
- Related to the former: concentration of quartiles at the beginning of the histogram. In other words, the cumulative distribution function goes up extremely quickly and then finds an elbow. This is particularly bad with the reference image.
- ANTs prefers distribution without large range (e.g., normalize the intensities to fit in a uint8 or int16).
This clipping tries that, but I haven't played a lot to set something robust and relatively data-driven.
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.
solid progress... how can I help more?
predicted, | ||
dwdata.affine, | ||
fixed, | ||
clip=reg_target_type == "dwi", |
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.
Yes, it's great you guys are bringing this up to the forefront.
Basically, the question here is that I have come to the conclusion that, even though ANTs has histogram equalization, it really needs help from the outside.
In general, there are a bunch of problems/situations where ants doesn't work very well:
- Presence of negative values (unless they are meaningufuly correct - e.g., if you are registering gradients)
- Presence of long tails (blood vessels on T1w images typically add very bright, but few, pixels which extend the histogram)
- Related to the former: concentration of quartiles at the beginning of the histogram. In other words, the cumulative distribution function goes up extremely quickly and then finds an elbow. This is particularly bad with the reference image.
- ANTs prefers distribution without large range (e.g., normalize the intensities to fit in a uint8 or int16).
This clipping tries that, but I haven't played a lot to set something robust and relatively data-driven.
terminal_output="file", | ||
from_file=pkg_fn( | ||
"eddymotion", | ||
f"config/dwi-to-{reg_target_type}_level{i_iter}.json", |
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.
As per discussion over email, we offline run registration.cmdline
and the massage the output CLI to replace image paths with ants images.
), | ||
fixed_image=str(fixed.absolute()), | ||
moving_image=str(moving.absolute()), | ||
**align_kwargs, |
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.
I'm leaning towards just storing one YAML file with the command lines that the estimator can pick.
src/eddymotion/utils.py
Outdated
if isinstance(val, str): | ||
if Path(val).exists() and (PurePosixPath(val).suffix == '.nii.gz' or PurePosixPath(val).suffix == '.nii'): | ||
return ants.from_nibabel(nb.load(val)) |
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.
unnecessary checks. Also, if there's a function to import from nibabel, we don't need to impose ".nii[.gz]" for the extension.
if isinstance(val, str): | |
if Path(val).exists() and (PurePosixPath(val).suffix == '.nii.gz' or PurePosixPath(val).suffix == '.nii'): | |
return ants.from_nibabel(nb.load(val)) | |
val = Path(val) | |
if not val.exists(): | |
raise FileNotFoundError(val) | |
return ants.from_nibabel(nb.load(val)) |
test/test_utils.py
Outdated
import ants | ||
|
||
|
||
def test_antsimage_from_path(): |
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.
def test_antsimage_from_path(): | |
def test_antsimage_from_path(datadir, tmp_path): |
test/test_utils.py
Outdated
# Generate and save test data to file | ||
# Send through antsimage_from_path | ||
# Check if ANTsImage, same as original data | ||
assert isinstance(result, ants.ANTsImage) |
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.
assert isinstance(result, ants.ANTsImage) | |
fixed = tmp_path / "b0.nii.gz" | |
dwdata = DWI.from_filename(datadir / "dwi.h5") | |
b0nii = nb.Nifti1Image(dwdata.bzero, dwdata.affine, None) | |
b0nii.to_filename(fixed) | |
assert isinstance(antsimage_from_path(fixed), ants.ANTsImage) |
src/eddymotion/estimator.py
Outdated
# execute ants command line | ||
result = registration.run(cwd=str(tmpdir)).outputs | ||
#registration_kwargs = ants_utils._int_antsProcessArguments(registration_kwargs) | ||
registration_ants = ants.registration(**registration_kwargs) |
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.
registration_ants = ants.registration(**registration_kwargs) | |
ants_cli = ants.registration(**registration_kwargs).cmline | |
# Now split ants_cli and replace paths with image objects | |
# this is a bit more convoluted because some paths will come between brackets and commas | |
args = [load_if_image(token) for token in ants_cli.split(" ")] |
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.
eventually we will get rid of nipype, but this could be useful for now
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.
Could you show us where the .cmline is coming from? Are you suggesting that we revert back to the nipype Registration() call instead of the current ants call?
After some more detailed profiling, we came to the conclusion that this does not provide speedup and potentially reduces flexibility relative to the nipype interface (because the Python API does not expose the full API of the CLI). Unless someone objects, I suggest that we close this PR. |
Addresses #89