Fixes a bug in NiftiImageData.cpp causing segfault #1226
Conversation
evgueni-ovtchinnikov
changed the title
evgueni-ovtchinnikov
changed the title
| @@ -220,7 +220,7 @@ void NiftiImageData<dataType>::construct_NiftiImageData_from_complex_im_real_com | |||
|
|
|||
| auto &it_in = in_sptr->begin(); | |||
| auto &it_out = out_sptr->begin(); | |||
| for (; it_in!=in_sptr->end(); ++it_in, ++it_out) | |||
| for (; it_out != out_sptr->end(); ++it_in, ++it_out) | |||
There was a problem hiding this comment.
not sure why this is. Aren't they the same size?
There was a problem hiding this comment.
no, the input image dimensions are (2, 256, 256) and the output (1, 256, 256) because the first dimension of a NiftiImageData object is z-dimension, which is 1 for both input and output images, but the input image is reconstructed from MR acquisition data with two repetitions.
There was a problem hiding this comment.
Are we (silently) constructing a single volume from a repeated volume? That sounds wrong. I would suggest that we have a chechk that dimensions are identical (e.g. via get_geometrical_info() ) and if not throw an error.
Where is the call then that you're trying to fix?
There was a problem hiding this comment.
Throwing an error to avoid segfault is not very helpful, I believe - I would rather issue a warning that the complex data contains more than one image, and only the first one is going to be used. @paskino which way would you prefer?
Where is the call then that you're trying to fix?
In Reg.NiftiImageData.construct_from_complex_image, if I understand your question correctly
There was a problem hiding this comment.
I'm trying to understand if this is a "conceptual" problem (i.e. what should we do with weird data) or a bug (i.e. we know how to handle the data, but the code doesn't do it).
What data do we stick into Reg.NiftiImageData.construct_from_complex_image where it throws this error? Is it a normal complex image, and If it is complex data with 2 repetitions, then I would argue that the output should be a real image with 2 repetitions, not silently throw away the 2nd repetition.
Unfortunately, this might be getting this into our problem of encoding MRI dimensions. From your description, I understand that data of a single slice with 2 repetitions is encoded as a 3D NiftiImageData. That in itself would be wrong (it should be a 4D image, with a single slice). And as I stated above, the construct_from functions really should check that all args have the same geometry. If they don't, an error should be thrown as it is invalid usage. Otherwise other crashes are unavoidable.
I might misunderstand this completely of course...
There was a problem hiding this comment.
Is it a normal complex image, and If it is complex data with 2 repetitions, then I would argue that the output should be a real image with 2 repetitions, not silently throw away the 2nd repetition.
That would require NiftiImageData to handle multiple images, and efficient implementation of such a functionality may take time. I would make a separate issue/PR for that when the need for it arises - the current quick fix just allows the computation of the resampler norm.
From your description, I understand that data of a single slice with 2 repetitions is encoded as a 3D NiftiImageData.
No, that was what my quick fix tried to avoid, as it would be wrong indeed.
|
@evgueni-ovtchinnikov you will have to merge |
|
so... the tests worked. Can't say I understand that as I needed #1228. oh well. |
Changes in this pull request
Trying to fix a bug in
NiftiImageData.cppthat causes segfault.Testing performed
Tested on the code provided by @paskino in #1225.
Related issues
Fixes #1225.
Checklist before requesting a review
Contribution Notes
Please read and adhere to the contribution guidelines.
Please tick the following: