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

[Tom Vercauteren]

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
>
>