[Insight-developers] InPlaceImageFilter::CanRunInPlace not virtual + Potential misunderstanding with InPlaceImageFilter::GetInplace

[Luis Ibanez]

Hi Tom,

Thanks for looking at this problem.

The patch looks good.

I particularly agree with:


* making virtual the method "CanRunInPlace()".

* and with replacing:
-  if (m_InPlace && (typeid(TInputImage) == typeid(TOutputImage)))
+  if (this->GetInPlace() && this->CanRunInPlace()
   (so that the virtual method "CanRunInPlace()" is used
     instead of the manual check for the same type in the images).


The comment that you added about the default behavior
of "CanRunInPlace()" makes sense, but it probably should
go in the documentation of CanRunInPlace() instead of
being in the SetInPlace()/GetInPlace() block. It may also
be worth emphasizing (in the doc of CanRunInPlace() )
that this method may be overloaded by derived classes.


Feel free to commit the patch,
and of course...

        watch the Dashboard      :-)


   Thanks


        Luis


--------------------------------------------------------------------------
On Thu, Apr 30, 2009 at 6:05 AM, Tom Vercauteren
<tom.vercauteren at m4x.org> wrote:
> Hi all,
>
> I have prepared a small patch that addresses the issue I mentioned but
> only does the following:
> - makes CanRunInPlace virtual
> - use CanRunInPlace and GetInPlace in lieu of m_InPlace and
> (typeid(TInputImage) == typeid(TOutputImage))
> - small enhancement of the documentation of Set/GetInPlace
>
> It does not add a new function to InPlaceImageFilter nor does it
> change the semantics of anything.
>
> Anybody against such a change? If not, I'm planning to commit it
> beginning of next week.
>
> Regards,
> Tom
>
> On Wed, Apr 29, 2009 at 15:48, Tom Vercauteren <tom.vercauteren at m4x.org> wrote:
>> Hi Karthik,
>>
>> Thanks for the feedback.
>>
>>> - GetInPlace() / SetInPlace() is an opportunity for the user to state if
>>> he'd like the filter should run in place.
>>
>> I indeed found this out. I just wanted to mention that I find the name
>> GetInPlace a bit confusing this it only relates to a user choice and
>> not an actual fact.
>>
>>> - CanRunInPlace() is an opportunity for the filter to state if it is capable
>>> of running in place.
>>
>> That is perfectly clear.
>>
>>> So one should check both...
>>>
>>> I wouldn't add a third option, it'd make things more confusing.
>>
>> I am not proposing to add a third option but to add a new function
>> that simply returns the results of checking both, e.g.
>>
>> virtual bool WillRunInPlace()
>>  {
>>  return (this->GetInPlace() && this->CanRunInPlace());
>>  }
>>
>> The other proposal was to change the semantics of GetInPlace to check
>> for both m_InPlace and this->CanRunInPlace() but this doesn't seem
>> possible from a backward compatibility point of view.
>>
>>
>>
>>> No reason why CanRunInPlace is not virtual, although you seem to have a case
>>> in point to change it motivated by compatibility rather than design.
>>
>> I think it would make sense to have CanRunInPlace be virtual from a
>> design point of view and not only for compatibility issues. Below are
>> two of my main motivations:
>>
>> 1) Right now CanRunInPlace only checks whether the input and output
>> type are the same. It would be interesting for some filter to run in
>> place only if other conditions are also met e.g. some parameter range
>> could work in place but another could not.
>>
>> 2) If CanRunInPlace can be overridden (i.e. becomes virtual) and some
>> small changes are made to InPlaceImageFilter, it becomes easier for a
>> user to correctly use InPlaceImageFilter. For example, let's look at
>> InPlaceImageFilter::AllocateOutputs:
>> http://www.itk.org/cgi-bin/viewcvs.cgi/Code/Common/itkInPlaceImageFilter.txx?root=Insight&sortby=date&view=markup
>>
>> Using my proposed WillRunInPlace function, what it basically does is:
>>
>> if ( this->WillRunInPlace() )
>>  {
>>  Use existing output
>>  }
>> else
>>  {
>>  Allocate new output
>>  }
>>
>> However the if is encoded as:
>>
>> if (m_InPlace && (typeid(TInputImage) == typeid(TOutputImage)))
>>
>> This makes it necessary to override AllocateOutputs if the subclass
>> needs more conditions to be able to run in place. Similarly
>> ReleaseInputs needs to be overridden.
>>
>> Let me know if this is still somewhat unclear.
>>
>> Regards,
>> Tom
>>
>>
>>
>>>
>>> Thanks
>>> --
>>> karthik
>>>
>>> On Wed, Apr 29, 2009 at 7:52 AM, Tom Vercauteren <tom.vercauteren at m4x.org>
>>> wrote:
>>>>
>>>> Hi all,
>>>>
>>>> I wrote a subclass of InPlaceImageFilter. Because of  the following
>>>> (now fixed) bug, http://www.itk.org/Bug/view.php?id=8672, my filter
>>>> can only run in place for ITK version strictly greater than 3.12. I
>>>> thus need a backward compatibility scheme.
>>>>
>>>> My first try, was to override InPlaceImageFilter::CanRunInPlace to
>>>> return false if the ITK version is below 3.13. However I realized that
>>>> InPlaceImageFilter::CanRunInPlace is not virtual which means I'd
>>>> rather stay off overriding it... Is there a reason why
>>>> InPlaceImageFilter::CanRunInPlace is not virtual?
>>>>
>>>> Also, I initially thought that, if InPlaceImageFilter::CanRunInPlace
>>>> returned false, then InPlaceImageFilter::GetInplace would also return
>>>> false. This is however not the case. It seems that I need to both
>>>> check for CanRunInPlace and GetInplace to know whether a filter will
>>>> actually run in place.
>>>>
>>>> This design seems a bit odd to me. Wouldn't it be better if
>>>> InPlaceImageFilter::GetInplace would return whether a filter will
>>>> actually run in place? Or maybe we can add a new virtual function to
>>>> do that, e.g. InPlaceImageFilter::WillRunInPlace.
>>>>
>>>> Currently, it seems that such a function would only be used in three
>>>> itk classes ( PasteImageFilter, CastImageFilter and
>>>> DenseFiniteDifferenceImageFilter) but I think it would be worth having
>>>> it.
>>>>
>>>> Thoughts?
>>>>
>>>> Regards,
>>>> Tom Vercauteren
>>>> _______________________________________________
>>>> Powered by www.kitware.com
>>>>
>>>> Visit other Kitware open-source projects at
>>>> http://www.kitware.com/opensource/opensource.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
>
> 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
>
>