[Insight-developers] transform internal types
Luis Ibanez
luis.ibanez at kitware.com
Tue May 18 11:38:38 EDT 2010
Hi Marius,
Please see my comments below.
Thanks
Luis
----------------------
On Mon, May 10, 2010 at 9:46 AM, <M.Staring at lumc.nl> wrote:
> Hi Luis,
>
>>
>> Hi Marius,
>>
>> Thanks for this deep exploration of the registration framework.
>>
>> As you probably know, the registration framework has gone
>> through a couple of reincarnations in which template
>> arguments have been trimmed down.
>
> I didn't know that. Looking into the cvs history I see that ImageToImageMetric and ImageRegistrationMethod were templated over the fixed and moving image since first commit, and itk::Optimizer had its template arguments removed some 8 years ago: a couple of years before I started using ITK :-)
>
Yeap, the Evolutionary event happened on April 2002 (Eight years ago).
You may find interesting to look at the "dead files" in ViewCVS:
http://www.itk.org/cgi-bin/viewcvs.cgi/Code/Algorithms/itkMeanSquaresImageToImageMetric.h?hideattic=0&root=Insight&view=log
>>
>> This is a particularly sensitive issue when it comes to
>> supporting Wrapping, so, it is unlikely that we will move in
>> the direction of adding back template parameters to the
>> registration framework, unless there is a really strong
>> motivation for doing so.
>
> Ok, I don't know anything about wrapping.
>
> But apart from wrapping, is it such a problem to add a template parameter? The proposed way is backwards compatible, and by default behaves as before.
>
I'm not the one that you would have to convince :-)
>> --
>>
>> It seems that your original motivation is to be able to pass
>> transform parameters to a GPU, and that having the array of
>> parameters to
>> use floats is a convenient way to go. Is that right ?
>
> My motivation is two-fold:
> 1) Annoyance, which can be a great stimulator for me :-)
> Before I dove into this, I expected it would be easily possible with the ITK to specify the internal precision type used during the registration. I only need float precision, so just use floats, right. So, apart from how this should be implemented, would you agree with the folowing (Freedom Of Choice):
>
> It should be possible to use float precision during registration, when this is desirable.
>
In practice this is not realistic.
Floating point operations will be executed in "double" anyways.
So, a "float" declaration is only facilitating your data transfer.
It will have no positive impact in the computation of the transform,
and minimal impact in the memory consumption (unless you instantiate
thousands of Transforms...)
> ?
>
> After finding out that it was currently not possible, I looked for ways how to get this working. The best solution so far is the one with the added template parameter and the typedef to get backwards compatibility. I don't say this is the best solution period, I am just interested in 'a solution'.
>
A more realistic solution is to have an adaptor class
that will transfer parameters between the itk Transform
and you GPU code.
> 2) The second motivation indeed comes from the GPU. We would like to transfer a GPU implementation of the GetValueAndDerivative() to the ITK framework. Currently we just have it working completely outside the ITK. For that we need to pass the transform parameters from the ITK (CPU) to the GPU (and back again). We are working with the B-spline deformable transform, so the number of parameters is not 12 but sometimes in the order 10^6. Although still this may or may not consume that much time, it is a completely useless operation and a waste of resources - I just don't like making bad code like that.
>
I agree with you in that it is a waste of resources.
It is however an insignificant waste of resources.
We are talking about less than a millisecond here...
(for a processor that runs at GigaHertzs)
The amount of time that it will take to convert 10^6
doubles to floats....
This is insignificant compared to the time that it will
take to compute the metric.
If you are concerned about optimization of resources,
there are much better places in the registration framework
were brain power could be invested for a higher return.
> Interleaved you will find answers to your questions by Tamas Farago, who developed the GPU code:
>
> ----------------------------------------------
>
>>
>> You seem to be concerned about the performance of such
>> copying could have in the run-time performance of the code.
>>
>>
>> The only transform for which the parameters are a large
>> array, are the BSpline deformable transforms and the Kernel
>> transforms.
>>
>> Before them, the largest array that we usually deal with is a
>> 3D Affine Transform, that comes down to 12 values.
>
> We are using the B-spline, with much more parameters.
>
Let's quantify:
A typical BSpline in 3D will have about 3 x 50^3 parameters
which is about ~ 375,000 parameters
round them up to 1^6 parameters.
This conversion takes 1.75 milliseconds in a 2.8Ghz processor.
>>
>> When you are passing transforms to the GPU, what is the
>> partition of labor between the CPU and the GPU ?
>
> GPU 80-85%, CPU 15-20%
>
And how much of this time is spent in actual data transfer ?
and from that time, what will be the % of change from using
float->float transfer or using float->double transfer ?
How many milliseconds are we talking about ?
>>
>> Are you performing a registration in the CPU and then
>> displaying outputs in the GPU ?
>
> No, the whole registration is performed on the GPU because of its parallel capabilities of processing the independent samples during a given iteration. Not for display.
>
How many milliseconds does it take to compute the
registration in the GPU ?
For what image size ?
>>
>> Are you performing registrations in the GPU ?
>
> Yes.
>
>>
>> In either case, it will be surprising if the conversion of 12
>> doubles to floats will be of any significant impact of
>> performance compared to the time that it takes to compute one
>> evaluation of an image metric.
>>
>> Even for the case of the BSpline transforms, the time that it
>> takes to evaluate the metric is orders of magnitude longer
>> than what it would take to copy from double to float the
>> array of Transform parameters.
>
> Yes, that is true, but seemingly copying the results back to the CPU every iteration causes this conversion to be applied twice double->float and float->double and is totally adverse to performance. And useless as a simple template parameter will fix the whole issue.
>
You are greatly underestimating the maintenance cost
of one extra template parameter.
and trying to fix a problem of minimal significance.
>>
>> Could you maybe illustrate the original problem ?
>
> The interpolator in the GPU used for B-spline and any other transforms is only capable of handling floats, thus the need for conversion.
>
>>
>> Have you found evidence of a run-time performance degradation ?
>
> No, there has been no benchmarking yet, the issue only came up during development. Judging from the ITK codebase the choice of a double transformarray is arbitrary, unnecessary and forces a design choice on the developer.
>
In the absence of benchmarking,
all this discussion is rather pointless.
Let's get hard numbers.
We need times in milliseconds, image size,... etc.
Otherwise this is a purely mental exercise trying
to solve a problem that may or may not be real.
(in the meantime we could dedicate resources to
solve other problems that are actually very real).
> ----------------------------------------------
>
> I will try to get some benchmark numbers soon.
>
> I hope I've clarified some points, and made a strong point for the need for choice of internal precision type.
> I don't really care how it gets implemented, but the solution proposed earlier still seems like a good candidate to me. Although I admit I cannot judge the impact on wrapping.
>
> Thanks for the feedback,
>
> Marius
>
>>
>>
>> Please let us know,
>>
>>
>> Thanks
>>
>>
>> Luis
>>
>>
>>
>> --------------------------------------------------------------
>> ---------------------
>> On Fri, May 7, 2010 at 4:49 AM, <M.Staring at lumc.nl> wrote:
>> > Hi again,
>> >
>> > My smart collegue Niels Dekker found a solution that is
>> fully backward
>> > compatible! And it does not require a CMake option.
>> >
>> > He proposed to create a TransformBaseTemplate<TParametersValueType>
>> > class and add a typedef for backwards compatibility:
>> >
>> > typedef TransformBaseTemplate<double> TransformBase;
>> >
>> > And then let itk::Transform inherit from TransformBaseTemplate,
>> > together with the addition of a fourth template parameter
>> > TParametersValueType, which defaults to double:
>> >
>> > Currently:
>> > template <class TScalarType,
>> > unsigned int NInputDimensions=3,
>> > unsigned int NOutputDimensions=3> class ITK_EXPORT
>> Transform
>> > : public TransformBase
>> >
>> > Proposed:
>> > template <class TScalarType,
>> > unsigned int NInputDimensions=3,
>> > unsigned int NOutputDimensions=3,
>> > class TParametersValueType = double> class ITK_EXPORT
>> > Transform : public TransformBaseTemplate<TParametersValueType>
>> >
>> > This way all code, like
>> > typedef itk::Transform<ScalarType,3,3> TransformType;
>> > TransformType::Pointer transform = TransformType::New(); still
>> > compiles, uses double as the internal precision type, i.e. works
>> > identical to the current situation.
>> >
>> > --------------------------
>> >
>> > To get this trick in the registration framework this also means
>> > changes at several other places:
>> > Change 1: adding a template parameter TParametersValueType
>> (defaulting
>> > to double) Change 2: adding a class CurrentClassTemplated +
>> a typedef
>> > CurrentClassTemplated<double> CurrentClass;
>> > - ImageToImageMetric, do Change 1
>> > - SingleValuedCostFunction, do Change 2
>> > - CostFunction, do Change 2
>> >
>> > - (MultiResolution)ImageRegistrationMetric, do Change 1
>> > - NonLinearOptimizer, do Change 2
>> > - Optimizer, do Change 2
>> >
>> > And probably some more places. The ResampleImageFilter
>> nicely already
>> > contains this trick:
>> >
>> > template <class TInputImage, class TOutputImage,
>> > class TInterpolatorPrecisionType=double>
>> > class ITK_EXPORT ResampleImageFilter: ...
>> >
>> > ----------------------------
>> >
>> > I created a patch that implements changes to TransformBase,
>> Transform,
>> > ImageToImageMetric, SingleValuedCostFunction and
>> CostFunction. You can
>> > inspect the patch in the attachment, so you can see what
>> the proposed
>> > changes precisely are. The experimental build reported no problems.
>> >
>> > How do the developers like this change ?
>> > Do you see any problems with it ?
>> >
>> > With kind regards,
>> >
>> > Marius
>> >
>> >
>> >> -----Original Message-----
>> >> From: insight-developers-bounces at itk.org
>> >> [mailto:insight-developers-bounces at itk.org] On Behalf Of
>> >> M.Staring at lumc.nl
>> >> Sent: donderdag 6 mei 2010 18:07
>> >> To: blowekamp at mail.nih.gov
>> >> Cc: insight-developers at itk.org
>> >> Subject: Re: [Insight-developers] transform internal types
>> >>
>> >> Hi Brad,
>> >>
>> >> Thanks for your response!
>> >>
>> >> >
>> >> > Hello,
>> >> >
>> >> > I am not sure making the TransformBase a tempted type is a
>> >> good idea.
>> >> >
>> >> > The registration framework uses this class as a polymorphic
>> >> interface
>> >> > to transforms. So that one interface can be used to manipulate
>> >> > multiple types. This is different then much of ITK which uses
>> >> > templated objects with out explicit virtual interfaces.
>> By making
>> >> > TransformBase have a templeted argument there is no
>> longer a single
>> >> > interface as TransformBase<double> is a different type then
>> >> > TransformBase<float> are different types!
>> >>
>> >> It seems that the registration framework uses the itk::Transform
>> >> instead of the itk::TransformBase. That class
>> >> (itk::Transform) is a templated class, but it can still be
>> used as an
>> >> interface in the registration framework for many derived
>> transforms.
>> >>
>> >> But, I guess it is easier in other cases to have a
>> non-templated base
>> >> class.
>> >>
>> >> >
>> >> > Your initial post indicated that you are trying to pass these
>> >> > arguments to the GPU, and that is the motivation for
>> >> needing floats.
>> >> > Perhaps it would be easier to use an adaptor design pattern
>> >> to change
>> >> > the interface of TransformBase, so that the array could be
>> >> converted
>> >> > when it needs to be uploaded to the GPU?
>> >>
>> >> That is indeed easier, but I like to circumvent the
>> explicit casting
>> >> and copying of potentially large arrays.
>> >>
>> >> A second reason is that the double precision of the
>> ParametersType is
>> >> often thrown away when specifying a float precision type for the
>> >> transform. So, why use double precision, if in the end you
>> don't, or
>> >> when it's not needed for an application.
>> >>
>> >> -----------------------------------
>> >>
>> >> I checked the image registration framework, and it seems I'm not
>> >> lucky:
>> >> The ImageToImageMetric defines
>> >>
>> >> typedef Transform<CoordinateRepresentationType,
>> >> MovDim,FixDim> TransformType;
>> >>
>> >> where
>> >>
>> >> typedef typename Superclass::ParametersValueType
>> >> CoordinateRepresentationType;
>> >>
>> >> which is again hard-coded to be a double in itk::CostFunction.
>> >> Apparently, the registration framework only works with an
>> >> itk::Transform<double,dim,dim>. So, even with the proposed
>> patch I'm
>> >> still not there, and I can't use the same trick as before
>> (with using
>> >> the ScalarType), since the ImageToImageMetric is only
>> templated over
>> >> fixed and moving image type.
>> >>
>> >> -----------------------------------
>> >>
>> >> The only (desperate) thing I can think of now is to define an
>> >> ITK_GLOBAL_INTERNAL_PRECISION_TYPE_IS_DOUBLE, which
>> defaults to true,
>> >> but can be set to false with cmake. Then do something like:
>> >>
>> >> #ifdef ITK_GLOBAL_INTERNAL_PRECISION_TYPE_IS_DOUBLE
>> >> typedef double ParametersValueType; #else
>> >> typedef float ParametersValueType; #endif
>> >>
>> >> in the TransformBase and something similar in
>> CostFunction, and maybe
>> >> other places in ITK.
>> >>
>> >> This way I can at least compile an ITK flavour with the required
>> >> precision.
>> >>
>> >> (The downside of this solution is that it is not easily
>> possible to
>> >> sometimes use float and sometimes use double within one program.)
>> >>
>> >>
>> >> I hope someone else has a better idea.
>> >>
>> >> >
>> >> > Good luck,
>> >> > Brad
>> >>
>> >> Thanks :-)
>> >>
>> >> Regards,
>> >>
>> >> Marius
>> >>
>> >> >
>> >> > On May 6, 2010, at 10:45 AM, M.Staring at lumc.nl wrote:
>> >> >
>> >> >
>> >> > Hi,
>> >> >
>> >> >
>> >> >
>> >> >
>> >> > However, backward compatibility problems occur:
>> >> >
>> >> >
>> >> > 1) Transform< float, indim, outdim > is changed wrt
>> >> > the
>> >> >
>> >> >
>> >> > ParametersType
>> >> >
>> >> >
>> >> > 2) there is no ParametersType typedef in
>> TransformBase
>> >> >
>> >> >
>> >> > anymore, so code that uses
>> >> > TransformBase::ParametersType does
>> >> >
>> >> >
>> >> > not work anymore.
>> >> >
>> >> >
>> >> >
>> >> > When I tried to really implement it, I got in trouble
>> >> with (2) moving
>> >> > the ParametersType typedef from TransformBase, because the
>> >> > TransformIO
>> >> > classes depend on it.
>> >> >
>> >> > So, as an alternative solution I added a template parameter
>> >> > TScalarType
>> >> > to the TransformBase, like
>> >> >
>> >> > #ifdef ITK_USE_TRANSFORM_SCALARTYPE_FOR_PARAMETERSTYPE
>> >> > template <class TScalarType = double >
>> >> > #endif
>> >> >
>> >> > and then do:
>> >> >
>> >> > #ifdef ITK_USE_TRANSFORM_SCALARTYPE_FOR_PARAMETERSTYPE
>> >> > typedef TScalarType
>> ParametersValueType;
>> >> > typedef Array< ParametersValueType > ParametersType;
>> >> > #else
>> >> > typedef double
> ParametersValueType;
>> >> > typedef Array< ParametersValueType > ParametersType;
>> >> > #endif
>> >> >
>> >> > I also had to make some small changes to TransformIO
>> >> classes. The
>> >> > complete patch is attached to this email. The good news
>> >> is that the
>> >> > experimental build succeeded.
>> >> >
>> >> > With kind regards,
>> >> >
>> >> > Marius
>> >> > <Insight_changes1.patch><ATT00001..txt>
>> >> >
>> >> >
>> >> > ========================================================
>> >> >
>> >> > Bradley Lowekamp
>> >> >
>> >> > Lockheed Martin Contractor for
>> >> >
>> >> > Office of High Performance Computing and Communications
>> >> >
>> >> > National Library of Medicine
>> >> >
>> >> > blowekamp at mail.nih.gov
>> >> >
>> >> >
>> >> >
>> >> >
>> >> _______________________________________________
>> >> Powered by www.kitware.com
>> >>
>> >> Visit other Kitware open-source projects at
>> >> http://www.kitware.com/opensource/opensource.html
>> >>
>> >> Kitware offers ITK Training Courses, for more information visit:
>> >> http://kitware.com/products/protraining.html
>> >>
>> >> Please keep messages on-topic and check the ITK FAQ at:
>> >> http://www.itk.org/Wiki/ITK_FAQ
>> >>
>> >> Follow this link to subscribe/unsubscribe:
>> >> http://www.itk.org/mailman/listinfo/insight-developers
>> >>
>> >
>> > _______________________________________________
>> > Powered by www.kitware.com
>> >
>> > Visit other Kitware open-source projects at
>> > http://www.kitware.com/opensource/opensource.html
>> >
>> > Kitware offers ITK Training Courses, for more information visit:
>> > http://kitware.com/products/protraining.html
>> >
>> > Please keep messages on-topic and check the ITK FAQ at:
>> > http://www.itk.org/Wiki/ITK_FAQ
>> >
>> > Follow this link to subscribe/unsubscribe:
>> > http://www.itk.org/mailman/listinfo/insight-developers
>> >
>> >
>>
>
More information about the Insight-developers
mailing list