SyneRBI / SIRF

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

NiftiImageData template arguments other than float don't compile #299

Closed johannesmayer closed 3 years ago

johannesmayer commented 5 years ago

@rijobro When an object of class NiftiImageData3D is constructed with a template argument not float one gets a compilation error:

#include "sirf/cReg/NiftiImageData3D.h"

int main( int argc, char *argv[] )
{   
    sirf::NiftiImageData3D< float > float_nifti;
    sirf::NiftiImageData3D< unsigned int > other_nifti;
}
In file included from /home/rich/Documents/Superbuild/Build/sources/SIRF/src/xDynamicSimulation/cDynamicSimulation/main.cpp:21:0:
/home/rich/Documents/Superbuild/Build/sources/SIRF/src/Registration/cReg/include/sirf/cReg/NiftiImageData.h: In instantiation of ‘sirf::NiftiImageData<dataType>::Iterator& sirf::NiftiImageData<dataType>::begin() [with dataType = unsigned int]’:
/home/rich/Documents/Superbuild/Build/sources/SIRF/src/xDynamicSimulation/cDynamicSimulation/main.cpp:67:1:   required from here
/home/rich/Documents/Superbuild/Build/sources/SIRF/src/Registration/cReg/include/sirf/cReg/NiftiImageData.h:445:22: error: no matching function for call to ‘sirf::NiftiImageData<unsigned int>::Iterator::Iterator(float*&)’
         _begin.reset(new Iterator(_data));
rijobro commented 5 years ago

Almost all of the classes in the cReg directory are templated, but explicit instantiation (I think that's the correct term) is given at the bottom of the .cpp files. E.g., for NiftiImageData3D, there is this line: https://github.com/CCPPETMR/SIRF/blob/51d131ef1b945560e131f5585df108ba744f6c2f/src/Registration/cReg/NiftiImageData3D.cpp#L104

If you want it to compile for other datatypes, you just need to stick in another line with your desired type. What type of numbers do you need? It should be fine for double, but I can't guarantee anything for trickier types, such as complex (or even int).

KrisThielemans commented 5 years ago

nope. Missing a template instantation would give a linking eror. this is a compilation error.

rijobro commented 5 years ago

Ok, looks like I didn't do the iterators correctly for other datatypes.

@johannesmayer This might be a very simple solution for you (if it suits your needs): In nifti_image, data are stored as a void*. This means that whenever you want to get your data back, there are often a lot of switch statements (e.g., in the NiftyReg source code). To avoid this, I decided to always store images as floats. So, you could potentially open your unsigned int image as a float. Internally, it will convert the image to float and will be converted back to unsigned int as you are saving (or can be saved to some other datatype).

Some of the example images in data/examples/Registration aren't originally float. E.g., running:

sirf_print_nifti_info test.nii.gz test2.nii.gz standard.nii.gz mouseFixed.nii.gz mouseMoving.nii.gz 

Printing info for 5 nifti image(s):
    ...
    datatype:          16, 16, 16, 16, 16
    ...
    orig_datatype:     4, 2, 16, 4, 4
    ...

So you can see that some are 4 (NIFTI_TYPE_INT16), one is 2 (NIFTI_TYPE_UINT8) and the last is 16 (NIFTI_TYPE_FLOAT32), but they all behave as expected.

Does that make sense? In short, just use NiftiImageData3D<float> even if the input data type is not float. On reflection, it is somewhat confusing...

johannesmayer commented 5 years ago

@rijobro Thanks for the suggestion. I tried, but using the constructor I need, i.e.

/// Construct from array 
NiftiImageData3D(const dataType * const data, const VoxelisedGeometricalInfo3D &geom)

, this does not work unfortunately since, then it takes the pointer to unsigned int and stores it as float. Internally this gets mixed up to something of the order e-44 instead of my unsigned ints.

rijobro commented 5 years ago

Strange. If you run sirf_print_nifti_info with your image, does it print 768 (NIFTI_TYPE_UINT32) for the original datatype?

rijobro commented 5 years ago

Ohhh, I see the problem. That particular constructor should probably be templated for input data type (unsigned int) and then resulting image type (float). That would solve you're problem, right?

johannesmayer commented 5 years ago

Yes, if I cannot use the template argument as unsigned int then this would probably be a working workaround.

rijobro commented 5 years ago

Ok, i've just pushed that to master. I've put in a ctest (for both NiftiImageData and NiftiImageData3D) in which I create an image from an unsigned int array.

https://github.com/CCPPETMR/SIRF/blob/ed219b4da022d4628df64d1f7769cd4c2c8a9e39/src/Registration/cReg/tests/test_cReg.cpp#L205-L222

rijobro commented 5 years ago

Can I close this?

KrisThielemans commented 5 years ago

@johannesmayer could you confirm if this is fixed?

johannesmayer commented 5 years ago

For me they don't compile. But I also have no idea why since I didn't dig into the code so unfortunately I cannot offer a potential solution.

However, the workaround Richard implemented is good for me and I don't need an instance of another type than float.

KrisThielemans commented 5 years ago

Thanks! I've postponed till later

rijobro commented 5 years ago

Would you be able to clarify what didn't compile? Thanks

johannesmayer commented 5 years ago

The same thing as I posted in the initial message of this thread. I thought you changed that there is a constructor to fill a NiftiImage<float> from a unsigned int*. This works for me, but as far as I understood didn't fix the problem with the Iterators?

rijobro commented 5 years ago

Ah yeah, ok. I don't plan on doing this any time soon though.

KrisThielemans commented 4 years ago

is this one still necessary? If so, should be rescheduled to 2.2?

KrisThielemans commented 3 years ago

we won't fix this at the moment as currently not required