[Insight-developers] The {Binary|Ternary}FunctorImageFilter problem.

[Bill Lorensen]

I was also a reviewer and to be honest I never looked at the patch. Bad Bill...

When I review a patch, here is the proces I follow.

1) git checkout master
2) git pullall
3) copy/paste gerrit patch link
4) git rebase master "the patch to be reviewed"
5) make

I always do a rebase to avoid clashing with master.

Bill

On Thu, Jun 2, 2011 at 11:37 AM, Luis Ibanez <luis.ibanez at kitware.com> wrote:
> Hi Kent,
>
> On Thu, Jun 2, 2011 at 10:49 AM, Williams, Norman K
> <norman-k-williams at uiowa.edu> wrote:
>> I feel like I saw I way to implement what Hans was asking for, and
>> foolishly I thought that adding the functionality near the base of the
>> inheritance hierarchy was actually a win -- it added new functionality to
>> a whole family of classes, without changing their behavior for existing
>> cases.
>>
>> It has turned into a big mess. Partly that was my fault, because I was
>> doing something that was a bit tricky internally.  I also pushed it up to
>> Master based on limited feedback and without much testing on compilers
>> aside from gcc.
>>
>
>
> I also have to share the blame here,
> since I approved that original patch
> and missed to see the implications
> that Brad L. pointed out yesterday.
>
>
>> So I guess what Luis is proposing is this:
>>
>> 1. Back out the changes I made. That means reversing this patch:
>>   http://review.source.kitware.com/#change,1725
>>
>
> Yes,
> This is what is being proposed in this patch:
> http://review.source.kitware.com/#change,1777
>
>
>> 2. Add space checking to the existing functions, at the
>>   {Binary|Ternary}FunctorImageFilter level. Throw exception
>>   if spaces don't match to a tolerance.
>>
>
> Yes,
>
> and... also
>
> a) Adding unit tests for these classes.
>
> b) Moving the UnaryFunctorImageFilter from ITK-Common
>     to ITK-ImageFilterBase so, they are all together in the
>     same directory (Unary,Binary,Ternary).
>
>
>> 3. Start a new, parallel tree of functor filters.  Probably needs to be
>>   in its own, new module?  What is it called?
>>
>
>
> Not sure about whether they need a new module,
> but certainly a new hierarchy is desirable.
>
>
>> Forgive me, Luis if I'm mis-representing what you've said.
>>
>
> You got it all right.
>
>> I also think Hans is right, in that the existing system behaves in
>> surprising, incorrect ways for medical imaging. Here at Iowa we have a lot
>> of experience with using and teaching ITK at the undergraduate, graduate
>> and professional levels. Part of the steepness of the learning curve is
>> avoiding pitfalls that arise from the pitfalls associated with physical vs
>> image space.
>>
>> But I want to be done with this controversy.  I can implement whatever
>> consensus solution is decided on.
>>
>>
>>
>> ________________________________
>> Notice: This UI Health Care e-mail (including attachments) is covered by the Electronic Communications Privacy Act, 18 U.S.C. 2510-2521, is confidential and may be legally privileged.  If you are not the intended recipient, you are hereby notified that any retention, dissemination, distribution, or copying of this communication is strictly prohibited.  Please reply to the sender that you have received the message in error, then delete it.  Thank you.
>> ________________________________
>> _______________________________________________
>> 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
>