[Insight-developers] RE: Changes to ImageSource::GraftNthOutput
Luis Ibanez
luis.ibanez at kitware.com
Thu Dec 1 16:42:03 EST 2005
Hi Jim,
1. Typos in the comments of Graft() method in DataObject.h
were fixed. Thanks for pointing this out.
2. You are right, the reasong for keeping the ImageBase::Graft()
method with the old signature was to respect backward
compatibility. However, as you pointed out, this may be
confusing for other developers maintaining this code in
the future. (see point 4.).
3. The reason why ImageSource should take a DataObject is that
there is no reason to assume that Ouput 2 is an image at all.
It could be any other object, specially now that the DataObject
Decorator is available.
For example, the itkStatisticsImageFilter could presumably
post the Mean as Output2, the standard deviation as Output3,
and so on.
4. The Graft() methods were changed in order to take only
"DataObject *" as argument. This will help to make the
code easily understandable. As you mention, the drawback
is that the dynamic cast is performed multiple times, but
in any case, grafting is not the kind of operation that
will be put in an inner loop. So, even if it is slow, it
will be neglieable compared to the execution time of any
image filter.
I'm running an experimental in [zion.kitware] with these changes,
and hopefully will be able to commit them before the closing of
the Dashboard tonight at 9:00pm.
Thanks
Luis
---------------------------------
Miller, James V (Research) wrote:
> Luis,
>
> Looked through the changes quickly.
>
> 1. There are some typos in the comments to DataObject.h
>
> 2. Should ImageBase keep both versions of Graft(), one that takes a
> DataObject and one that takes an ImageBase. I suppose keeping them
> both maintains backward compatibility.
>
> 3. In ImageSource, you made the opposite decision, changing the function
> to take an DataObject instead of taking an ImageBase.
>
> 4. Should Image::Graft() also be changed to take a DataObject? I understand
> why the code works without changing Image::Graft() but it not obvious.
> (ImageSource::Graft(DataObject*) calls the virtual function from
> DataObject::Graft(DataObject*) which gets mapped ImageBase::Graft(DataObject*) which calls
> the virtual function ImageBase::Graft(ImageBase*) which gets mapped to Image::Graft(ImageBase*) ).
>
> This may confuse the innocent. Maybe all versions of Graft() should take a DataObject*. The
> only downside to this is that it will be dynamic_cast'ed multiple times.
>
>
> Jim
>
> -----Original Message-----
> From: Luis Ibanez [mailto:luis.ibanez at kitware.com]
> Sent: Thursday, December 01, 2005 11:16 AM
> To: Miller, James V (Research)
> Cc: Luis Ibanez; Insight Developers List; Lorensen, William E (Research)
> Subject: Re: [Insight-developers] RE: Changes to
> ImageSource::GraftNthOutput
>
>
>
> Jim,
>
> The fixes for the Graft methods were committed this morning:
>
> http://www.itk.org/Testing/Sites/dash12.kitware/Win32-vs70/20051201-1219-Continuous/Update.html
>
>
> They seem to be doing ok in the Continuous builds.
>
>
> Unless there are any objections, I will commit the same
> fixes on to the ITK 2.4 branch and update the tarball files.
>
>
> Please let me know if you see any reason for not patching
> the branch.
>
>
> Thanks
>
>
>
> Luis
>
>
>
> ---------------------
> Luis Ibanez wrote:
>
>>Hi Jim,
>>
>>The solution that you suggested seems to be the right way to go.
>>
>>I'm testing that implementation in an Experimental build, and
>>if it looks good I'll commit it tonight.
>>
>>
>> Thanks
>>
>>
>> Luis
>>
>>
>>------------------------------------
>>Miller, James V (Research) wrote:
>>
>>
>>>Luis,
>>>I still don't think the dynamic_cast works since the image pointer was
>>>already
>>>static_cast'ed to TOutputImage*. Once cast to that, I don't think the
>>>dynamic_cast
>>>will be able to determine whether the image was really of a different
>>>type.
>>>
>>>The implementation of Graft() in ImageBase and Image do take some
>>>these things
>>>into consideration. The argument to Graft() in ImageBase and Image is a
>>>ImageBase of the same dimension as the image you are grafting onto.
>>>So the output and graft image must be instances of an ImageBase with
>>>the same dimension. Unfortunately, due to the static_cast to
>>>TOutputImage*, this
>>>is probably always true.
>>>
>>>In Image::Graft(), the grafting of the pixel data will only occur if
>>>the graft image has the same type of pixel as the image being grafted
>>>onto. This is
>>>what we want.
>>>
>>>What would be ideal, is if ImageSource::GraftNthOutput() could take an
>>>ImageBase
>>>that is correct dimension for the output being grafted onto. I can't
>>>think of a way to do this.
>>>
>>>Thus, I think the GraftNthOutput() method and the GraftOutput() method
>>>of ImageSource
>>>should probably take a DataObject*.
>>>Ideally, we would then want to cast this DataObject to an ImageBase of
>>>the appropriate dimension
>>>for the output being grafted. If that case was successful, then we
>>>could call the the Graft method in Image. Unfortunately, I don't think
>>>there is a way to do this.
>>>
>>>This leaves us with also having to change the arguments to
>>>ImageBase::Graft() and Image::Graft() to take
>>>a DataObject*. Each of these implementations would do a dynamic cast
>>>to what they are expecting
>>>(ImageBase::Graft would dynamic_cast to an ImageBase of the
>>>appropriate dimension, Image::Graft
>>>would dynamic_cast to an Image of the appropriate dimension AND pixel
>>>type).
>>>
>>>We should probably call ProcessObject::GetOutput() to get the data as
>>>an DataObject* (avoids
>>>a static_cast to a TOutputImage*).
>>>
>>>ImageSource<TOutputImage>
>>>::GraftNthOutput(unsigned int idx, DataObject *graft)
>>>{
>>> DataObject * output = this->ProcessObject::GetOutput(idx);
>>> output->Graft( graft );
>>>}
>>>
>>>
>>>-----Original Message-----
>>>From: Luis Ibanez [mailto:luis.ibanez at kitware.com]
>>>Sent: Wednesday, November 30, 2005 11:24 AM
>>>To: Miller, James V (Research)
>>>Cc: Lorensen, William E (Research); Insight Developers List
>>>Subject: Re: Changes to ImageSource::GraftNthOutput
>>>
>>>
>>>
>>>
>>>Hi Jim,
>>>
>>>
>>>I'm looking at the changes on the ImageSource::GraftNthOutput() method
>>>and found that the code was already assuming that all the outputs are
>>>of the same type.
>>>
>>>
>>>
>>>Please take a look at the differences between the current version and
>>>the previous one:
>>>
>>>http://www.itk.org/cgi-bin/viewcvs.cgi/Code/Common/itkImageSource.txx?root=Insight&r1=1.59&r2=1.60
>>>
>>>
>>>
>>>
>>>The previous version (1.59) was doing roughtly the following:
>>>
>>>
>>>ImageSource<TOutputImage>
>>>::GraftNthOutput(unsigned int idx, TOutputImage *graft)
>>>{
>>> typename ImageSource<TOutputImage>::OutputImageType * output
>>> = this->GetOutput(idx);
>>>
>>> output->Graft( graft );
>>>}
>>>
>>>
>>>
>>>So:
>>>
>>>
>>>
>>>1) The signature of GraftNthOutput() itself is already assuming
>>> that the argument to be grafted is of type "TOutputImage".
>>> Which is incorrect for filters having multiple output images
>>> of different types.
>>>
>>>
>>>2) It takes the DataObject returned by GetOutput( idx ) and
>>> statically casted (without using static_cast<>) into a raw
>>> pointer to TOutputImageType. Again, assuming that the outpu
>>> idx, is actually of type TOutputImageType *. This is also
>>> incorrect for filters with multiple output images of different
>>> types.
>>>
>>>
>>>3) It passes the second argument (of type TImageOutput *)
>>> to the Graft() method the "output" variable.
>>>
>>>
>>>In this context, the addition of the dynamic_cast was appropriate,
>>>because it is actually verifying that the output is of the
>>>correct type, instead of forcefully casting it without verification.
>>>
>>>
>>>
>>>It seems that what should be fixed is the type itself of the
>>>second argument of GraftNthOutput(), which should probably be
>>>just a (DataObject *) in order to be as general as the
>>>GetOutput() method.
>>>
>>>
>>>The code should look like:
>>>
>>>
>>>ImageSource<TOutputImage>
>>>::GraftNthOutput(unsigned int idx, DataObject *graft)
>>>{
>>> DataObject * output = this->GetOutput(idx);
>>> output->Graft( graft );
>>>}
>>>
>>>
>>>Which also means that the Graft() method should be declared
>>>virtual at the level of the DataObject, and all subsequent
>>>derived classes should reimplement it with an internal
>>>dynamic_cast that will verify if the argument DataObject*
>>>is actually pointing to the expected data type.
>>>
>>>
>>>Please let me know if you find these changes to
>>>be appropriate, and if so, I will commit then to
>>>the trunk and to the release branch.
>>>
>>>
>>>
>>> Thanks
>>>
>>>
>>> Luis
>>>
>>>
>>>
>>>
>>>---------------------------------
>>>Miller, James V (Research) wrote:
>>>
>>>
>>>>Luis,
>>>>You made some changes to GraftNthOutput in ImageSource that include
>>>>throwing exceptions and performing a dynamic_cast.
>>>>
>>>>Why was the dynamic_cast change necessary? The output being grafted
>>>>does not have to be same type as output 0. If we are worried about
>>>>the types, then it really has to be the same type as the output being
>>>>grafted (indicated by idx) not output 0. May have to check the
>>>>typeid of graft and the selected output.
>>>>
>>>>I am not sure the dynamic_cast as written will do anything. The
>>>>return type of GetOutput() has already been static_cast to be the
>>>>same output 0 anyway.
>>>>
>>>>Jim Miller _____________________________________
>>>>Visualization & Computer Vision
>>>>GE Research
>>>>Bldg. KW, Room C223
>>>>1 Research Circle, Schenectady NY 12309-1027
>>>>
>>>>millerjv at research.ge.com <mailto:millerjv at research.ge.com>
>>>>(518) 387-4005, Dial Comm: 8*833-4005
>>>>Cell: (518) 505-7065, Fax: (518) 387-6981
>>>>
>>>>
>>>>
>>>>
>>>
>>>
>>>_______________________________________________
>>>Insight-developers mailing list
>>>Insight-developers at itk.org
>>>http://www.itk.org/mailman/listinfo/insight-developers
>>>
>>>
>>
>>_______________________________________________
>>Insight-developers mailing list
>>Insight-developers at itk.org
>>http://www.itk.org/mailman/listinfo/insight-developers
>>
>>
>
>
>
>
More information about the Insight-developers
mailing list