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

Add reg_f3d2 support #943

Draft
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

ALEXJAZZ008008
Copy link
Member

@ALEXJAZZ008008 ALEXJAZZ008008 commented Jun 10, 2021

reg_f3d2 works, however currently it flips the output by 180 degrees. If an affine transformation is given as input it does not flip, weirdly.

Insight on the flipping would be appreciated, I'm not certain if the issue is in SIRF or NiftyReg.

@ALEXJAZZ008008
Copy link
Member Author

ALEXJAZZ008008 commented Jun 10, 2021

FYI, addresses #944 for completeness.

@ALEXJAZZ008008
Copy link
Member Author

ALEXJAZZ008008 commented Jun 10, 2021

Furthermore to what I mention above, I should mention that I've tested NiftyReg on the command line; I don't see the flip with -vel (but I do with -sym, as expected from KCL-BMEIS/niftyreg#71). In NiftyReg, reg_f3d2 inherits from reg_f3d_sym so it may be assumed that the issue comes from reg_f3d_sym, however people using reg_f3d2 not through SIRF don't see this issue (and I mentioned I tested it on the command line and couldn't reproduce).

I can produce the issue with:

import sirf.Reg as reg


nifty_f3d_sym = reg.NiftyF3dSym()

nifty_f3d_sym.set_reference_image(reg.NiftiImageData3D("./ref.nii"))
nifty_f3d_sym.set_floating_image(reg.NiftiImageData3D("./flo.nii"))

nifty_f3d_sym.process()

resampler = reg.NiftyResample()

nifty_f3d_sym.get_deformation_field_forward().write("./dvf.nii")

resampler.set_reference_image(reg.NiftiImageData3D("./ref.nii"))
resampler.set_floating_image(reg.NiftiImageData3D("./flo.nii"))
resampler.add_transformation(nifty_f3d_sym.get_deformation_field_forward())

resampler.set_interpolation_type_to_cubic_spline()

resampler.forward(reg.NiftiImageData3D("./flo.nii")).write("./output.nii")

I do not see the issue with:

import sirf.Reg as reg


nifty_f3d_sym = reg.NiftyF3dSym()

nifty_f3d_sym.set_reference_image(reg.NiftiImageData3D("./ref.nii"))
nifty_f3d_sym.set_floating_image(reg.NiftiImageData3D("./flo.nii"))

nifty_aladin_sym = reg.NiftyAladinSym()

nifty_aladin_sym.set_reference_image(reg.NiftiImageData3D("./ref.nii"))
nifty_aladin_sym.set_floating_image(reg.NiftiImageData3D("./flo.nii"))

nifty_aladin_sym.process()

nifty_f3d_sym.set_initial_affine_transformation(nifty_aladin_sym.get_transformation_matrix_forward())

nifty_f3d_sym.process()

resampler = reg.NiftyResample()

nifty_f3d_sym.get_deformation_field_forward().write("./dvf.nii")

resampler.set_reference_image(reg.NiftiImageData3D("./ref.nii"))
resampler.set_floating_image(reg.NiftiImageData3D("./flo.nii"))
resampler.add_transformation(nifty_f3d_sym.get_deformation_field_forward())

resampler.set_interpolation_type_to_cubic_spline()

resampler.forward(reg.NiftiImageData3D("./flo.nii")).write("./output.nii")

or:

import sirf.Reg as reg


nifty_f3d_sym = reg.NiftyF3dSym()

nifty_f3d_sym.set_reference_image(reg.NiftiImageData3D("./ref.nii"))
nifty_f3d_sym.set_floating_image(reg.NiftiImageData3D("./flo.nii"))

nifty_f3d_sym.get_deformation_field_forward().write("./dvf.nii")
nifty_f3d_sym.get_output().write("./output.nii")

Thus is the problem with the resampler? Would you expect the issue to be resolved if you give an input affine transformation if the issue was the resampler?

@ALEXJAZZ008008
Copy link
Member Author

Does #832 have anything to do with it? I have pulled master.

@ALEXJAZZ008008
Copy link
Member Author

Example code and results: resample is the first code snippet from above, affine is the second code snippet from above and no_resample is the third code snippet from above.

resample: https://drive.google.com/file/d/1vubD6Lx6m7ao_zLILxJ-Ae_stBfFVOW4/view?usp=sharing
affine: https://drive.google.com/file/d/1pQgO8ij2qOBttdFZ0IC3SjM0ZE3qsYkz/view?usp=sharing
no_resample: https://drive.google.com/file/d/1ZOAThIfLD7cG_L0D88hhjFzVUDvypwpe/view?usp=sharing

Alternatively: https://github.com/ALEXJAZZ008008/SIRF_niftyreg_f3d2_test

@ALEXJAZZ008008 ALEXJAZZ008008 changed the title Add niftyreg_f3d2 support Add nifty_f3d2 support Jun 10, 2021
@ALEXJAZZ008008 ALEXJAZZ008008 changed the title Add nifty_f3d2 support Add reg_f3d2 support Jun 10, 2021
@KrisThielemans KrisThielemans linked an issue Jun 11, 2021 that may be closed by this pull request
@evgueni-ovtchinnikov evgueni-ovtchinnikov added this to the v3.2 milestone Jun 17, 2021
is compose needed in input is less than one
get def from cpp needed to get from spline then vel due to f3d2
@KrisThielemans
Copy link
Member

what's the status of this one? Should we review it again?

src/Registration/cReg/NiftyF3dSym.cpp Outdated Show resolved Hide resolved
nifti_image* init_cpp;

// If there is an initial transformation matrix, set it
if (_initial_cpp_sptr) {
Copy link
Member

Choose a reason for hiding this comment

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

I guess we should check if both are present and then throw an error that we cannot handle it

Comment on lines +69 to +72
reg_spline_getDefFieldFromVelocityGrid(cpp.get_raw_nifti_sptr().get(),
this->_nifti_image.get(),
false // the number of step is not automatically updated
);
Copy link
Member

Choose a reason for hiding this comment

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

is this correct? create_from_cpp always call this?

Copy link
Member Author

Choose a reason for hiding this comment

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

This isn't correct, this will only work with f3d2, I wrote it this way for speed of implementation. Really we should check to see if _use_velocity is set

Copy link
Member Author

Choose a reason for hiding this comment

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

The problem being that _use_velocity is a member variable of NiftyF3dSym

Copy link
Member

Choose a reason for hiding this comment

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

not sure what you mean, but maybe this just needs 2 different functions create_from_cpp and create_from_velocity_field. Or it needs some header inspection

Copy link
Member

@KrisThielemans KrisThielemans left a comment

Choose a reason for hiding this comment

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

Let's convert this to Draft at the moment, till you're done.

By the way, for whatever reason the PR removed requirements.txt

@evgueni-ovtchinnikov evgueni-ovtchinnikov modified the milestones: v3.6, v3.7 Dec 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add reg_f3d2 support
3 participants