SyneRBI / SIRF

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

VoxelisedGeometricalInfo should check its axes are valid #371

Open ashgillman opened 5 years ago

ashgillman commented 5 years ago

I was just looking through the code while writing up some info on the geometrical info class, and realised:

https://github.com/CCPPETMR/SIRF/blob/master/src/common/include/sirf/common/GeometricalInfo.h#L127

The initialiser for this class should make sure that each column of the direction matrix is a vector with magnitude 1 (each axis direction is a unit vector), and that the absolute value of the determinant should be (approximately) 1 (the axes must be perpendicular).

rijobro commented 5 years ago

Absolutely. These checks are done elsewhere in SIRF so this should be a simple copy and paste job.

There's still something that trips me up, however. The majority of images treated in SIRF don't need the VoxelisedGeometricalInfo. The only time we need it is when we change from one modality to another. Hence, we shouldn't throw an error if this fails.

Perhaps we print a warning if it fails and set a bool (e.g., set_up_successful) to false.

ashgillman commented 5 years ago

Oh they are? Perhaps those calculations should be moved here, rather than copied.

Hmm, do you think? What if we, for example, were to decide that image addition should first verify that the two images have the same geom info? Something like that could save future difficult-to-debug issues.

It depends how images are going to be generated I guess. Wouldn't you generally read a template image, that the data would be populated from? But I would argue that regardless, those properties should always be met.

KrisThielemans commented 5 years ago

I also vote for having checks for proper geo-info (and other things) in the construction stage. What is the use of an image without geo-info for anything to do with reconstruction or registration?

rijobro commented 5 years ago

I also vote for having checks for proper geo-info (and other things) in the construction stage.

Think we all agree on this.

What if we, for example, were to decide that image addition should first verify that the two images have the same geom info?

Yes, definitely. However for example, the NIfTI format can use the q- or s-form code. The s-form will get it into a standard space, whereas the q-form will get it into scanner space. What should we do if a NIfTI image only contains an s-form code? We'd have no way of comparing it to other images in scanner space (e.g., for addition).

DANAJK commented 5 years ago

Checks at constructor stages are good. The actual code can be tricky* so it should not be repeated elsewhere (though the checks might be called again after some processing). It can be necessary to update geom info for an existing dataset, for example after FFT, there may be a need to chop or zero pad an image, or flip/rotate to radiological presentation and this should result in a change to the geom info associated with the image.

For datasets that are simulated, I recommend the constructor does not allow a default geometry without the user explicitly asking. For example the user could pass a flag such as 'default_axial' and the constructor would set geom info for axial slices, but the key point here is that the user has to request this - otherwise there may be too much happening in the background leading to errors/misunderstandings down the line.