SyneRBI / SIRF

Main repository for the CCP SynerBI software
http://www.ccpsynerbi.ac.uk
Other
57 stars 29 forks source link

NiftyResampler norm crashes on MR data with multiple repetitions #1225

Closed paskino closed 6 months ago

paskino commented 7 months ago

With https://github.com/SyneRBI/SIRF/pull/1186 norm on the NiftyResampler was introduced. While running the MCIR reconstruction I call it and python crashes.

A minimal example to reproduce it is


import sirf.Reg as pReg
import sirf.Gadgetron as pMR
import numpy as np
import os

from cil.utilities.display import show2D

from sirf.Utilities import  examples_data_path
acq_data = pMR.AcquisitionData(os.path.join
            (examples_data_path('MR'),'simulated_MR_2D_cartesian.h5')
        )
# ad1.sort()
preprocessed_data = pMR.preprocess_acquisition_data(acq_data)
recon = pMR.FullySampledReconstructor()
recon.set_input(preprocessed_data)
recon.process()
im_ms = recon.get_output()

max_shift = 20.

t_x=max_shift
t_y=0
tm = pReg.AffineTransformation(np.array(\
        [[1, 0, 0, t_x], \
            [0, 1, 0, t_y], \
            [0, 0, 1, 0  ], \
            [0, 0, 0, 1  ]]))

im_ms1 = im_ms * 1.1

# Create resampler
mf_resampler = pReg.NiftyResample()
# mf_resampler[ind] = NiftyResampleWithNormCache()
mf_resampler.set_reference_image(im_ms)
mf_resampler.set_floating_image(im_ms1)
mf_resampler.add_transformation(tm)
mf_resampler.set_padding_value(0)
mf_resampler.set_interpolation_type_to_linear()

mf_resampler.norm()

I am running this on SIRF from the https://github.com/SyneRBI/SIRF/tree/ignore-acq branch

evgueni-ovtchinnikov commented 7 months ago

@paskino im_ms needs to be NiftiImageData, so after im_ms = recon.get_output() add

im_ms = pReg.NiftiImageData(im_ms)
KrisThielemans commented 6 months ago

Notes after discussion with @evgueni-ovtchinnikov.

Root-cause is #1044. The input data in the example has 2 repetitions. NiftyResampler::norm constructs a NiftiImageData from the MRImageData. The NiftiImageData has effectively only 1 repetition, but all the image-data gets copied into it, therefore causing buffer overflow and segfault. More notes added in #1044.

evgueni-ovtchinnikov commented 6 months ago

Just what I was afraid of - repetition is not the only extra dimension:

MR data have many different dimensions, obviously 3 spatial ones, but the additional ones (at least defined in ISMRMRD) are:

contrast phase repetition set

and I have just remembered David mentioning 11 dimensions.

Makes the situation still more complicated...

DANAJK commented 6 months ago

I would argue that trying to squash everything into 3 dimensions is more complicated, and more confusing, than using multiple array dimensions. I made comments about this in Feb 2022.