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

[Tom Vercauteren]

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
>>
>>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: itk-inplace.patch
Type: text/x-diff
Size: 3246 bytes
Desc: not available
URL: <http://www.itk.org/mailman/private/insight-developers/attachments/20090430/57fc3ed0/attachment.patch>