Reviews

Tom Vercauteren

Monday 15 October 2007

Summary:
The authors propose a new image class that takes into account an orientation matrix in addition to a diagonal spacing matrix and an origin point. The aim is to optimize the conversion of points and gradients between pixel space and physical space in the most commonly used cases. The focus is on providing a solution that is generic enough to cover all possible cases. This topic is very important for registration problems where the conversion can be needed millions of times. The solution proposed by the authors is based on function pointers.

Overall I think it is a good approach. I would suggest using this idea in the basic itk::Image class and deprecate the itk::OrientedImage class.

Hypothesis:
I fully agree with the hypothesis made by the authors:
1) Most ITK users will typically use images with a simple combination of orientation matrix, spacing vector and origin
2) Many users will need non trivial orientation matrices
3) All users care about the performance of registration

Evidence:
The authors claim that their solution:
1) is as generic as the oriented image class
2) can deal with the orientation of gradients
3) is often even more efficient than the standard image class
55 timing tests are provided to support the author claims.

Open Science:
The paper fully adheres to the open science concept. The code, the testing framework and the data are provided. The code uses only ITK.

Reproducibility:
I have been able to reproduce the authors work. The only thing I needed to change was a #define SPEEDTESTNUM which by default was set to a ridiculously low value and did not allow me to compare the time taken by the different implementations. On my platform (ubuntu, x86, gcc 4.1.2), the authors fast implementation compared even better with respect to the standard image class.

A very small README file would be useful to tell the reader what the log files refer to.

Use of Open Source Software:
The work uses ITK. The author give a brief comparison of their approach to gradient orientation problems with the one being proposed by the ITK team.

Open Source Contributions:
It took me very little time to compile and run the tests.

Code Quality:
The code needs a little cleanup to give it a consistent indentation, move out the #define printouts and so on. In itkFastOrientedImage.h a few #include are nor necessary.

Applicability to other problems:
The idea of using function pointers to use specialized optimized functions could be used in many ITK classes. A lot of filters could be optimized if, during their initialization, a function pointer was set to point to an efficient specialized function.

Suggestions for future work:
As mentioned by the authors, the use of function pointers forbids the compiler to inline the function. This has a performance price. The author claim that using specialized template classes would lead to an even better performance. This approach is however less generic than the previous one as it has to be decided at compile-time rather than run-time. For some users, compile-time may be fine. It would thus be great to include for example a very basic image class that only allows for unit orientation, unit spacing and zero origin. This would prove the author claim and let the user decide whether the added performance is worth the loss of genericness.

Many specific cases are not addressed. If the orientation matrix (including spacing or not) is composed of only 1s and -1s, optimized version can also be designed. It would be nice to have all those specialized versions, even though it becomes tricky :)


Additional Comments:
In the current constructor, the orientation and spacing are set to the identity and the origin is set to zero but the function pointers are set to the generic cases. I would be more consistent and make more sense to point to the specialized version.

The authors claim that "there is no point implementing a set of functions for TransformPhysicalPointToIndex". At first this puzzled me. It would be clearer to say something like "there is no point implementing a set of specialized functions for TransformPhysicalPointToIndex because the generic one cannot really be improved."

In the code, I saw:
// cast to integer is really really slow... maybe rewrite in assembly later.
#define RealToInt(A) static_cast(A)
A first improvement may come from using vcl functions. Then it might be worth looking at how this issue is addressed in the BSplineInterpolator. See http://www.itk.org/Bug/view.php?id=2078 for a discussion of its accuracy.