[Insight-developers] Bug in itkShrinkImageFilter

[Bill Lorensen]

Hans,

I agree that #3 is the best alternative as long as the API is not
changed. I think that improved results that fix a bug do not violate
backward compatibility.

BTW: We had a huge ice storm in our area. My son lives near Kitware
and does not expect power until Sunday or Monday. The mailing list and
repositories will probably be down.

Bill

On Fri, Dec 12, 2008 at 11:06 AM, Hans Johnson <hans-johnson at uiowa.edu> wrote:
> Bill,
>
> I tried to add these attachments to the bug reporting system, but that
> appears to be down.
>
> I've created a new class ( MultiResolutionPyramidResampleImageFilter ...
> NOTE Resample in name) for creating an image pyramid that preserves the
> physical locations of objects.  This class uses the ResampleImageFilter like
> you suggested.
>
> ---  I am now in need of some more advice on "Bug Fix" vs. "Preserve
> Backwards Compatibility".   I see 3 options in order of increasing
> intrusiveness:
>
> Quietly add this new filter to ITK so that it is available, add new test,
> but make no other changes
> Place the old filter in depracated mode, update existing test to use new
> filter
> Assert that this is a nasty bug and update current filter and current tests
> with the bug fix.
>
> My preference is option #3, but that means that some results will be
> changed.  I think that this is consistent with the decision that oriented
> images should be the default because this it was a bug fix, and now the
> changed results are consistent with the claims of how ITK interprets
> physical coordinates.
>
> Please let me know what path to take.
>
> Thanks,
> Hans
>
> PS:  My optimization of new registration method parameters across image
> scales is MUCH more efficient (about 1.5x faster) now that the physical
> space is not changing across scales!
>
> On 12/10/08 11:19 AM, "Bill Lorensen" <bill.lorensen at gmail.com> wrote:
>
> Hans,
>
> I'll look at the ShrinkImage problem and also look at Brad's bug.
>
> Bill
>
> On Wed, Dec 10, 2008 at 11:20 AM, Hans Johnson <hans-johnson at uiowa.edu>
> wrote:
>> Bill,
>>
>> THANKS!  These comments are exactly what I was hoping for.
>>
>> So this now becomes two separate issues:
>> 1) Change multiresolution pyramid filter to use ResampleImageFilter.
>> 2) Make the Shrink image filter preserve object physical locations better,
>> but without using interpolation.
>>
>> ====================
>>
>> I'll be working on a solution to #1 above today, but since this eliminates
>> my immediate need to solve problem #2 with the ShrinkImageFilter I'll
>> likely
>> not be addressing that problem anytime soon.  Could that be assigned to
>> someone else?
>>
>> ====================
>> With regards to point number 2 above:
>>
>> I feel quite strongly that an 8x8 white square in the middle of a 32x32
>> space should retain it's center of mass after the shrink image is applied
>> with a shrink factor of 2,4, or 8.  This is one of the test cases in the
>> new
>> failing test I added last night.  In the current implementation, the
>> center
>> of mass is not preserved.
>>
>> In the case of shrinking a 64x64 image to an 8x8 image, the index
>> locations
>> map as follows
>>
>> Fill     Current    Proposed    Object Space Preserving
>> Output   Input      Input       Input ContinuousIndex
>> [0,0]    [0,0]      [4,4]       [3.5,3.5]
>> [1,1]    [8,8]      [12,12]     [11.5,11.5]
>> ...
>> ...
>> [7,7]    [56,56]    [60,60]     [59.5,59.5]
>>
>> By reading from the current input there is a bias towards shifting the
>> objects towards the origin in the current implementation.  Perhaps the
>> best
>> way to deal with this would be to add the difference between columns 3 and
>> 4
>> in the table above to the origin.
>>
>> * My proposed solution will need to better checking of boundary conditions
>> (seg fault would occur on images of size 60x60 being sub-sampled to and
>> image space of 8x8).
>>
>>
>> Thanks,
>> Hans
>>
>> On 12/10/08 7:21 AM, "Bill Lorensen" <bill.lorensen at gmail.com> wrote:
>>
>>> Hans,
>>>
>>> Another issue if you make your changes. If the new Shrink is run on a
>>> labeled image, I believe your code will interpolate the new pixels.
>>> This behavior is not desirable for subsampling a labeled image.
>>>
>>> Perhaps we should leave this filter alone and change the
>>> multiresolution pyramid code to use ResampleImageFilter with the
>>> proper values set to do subsampling.
>>>
>>> Bill
>>>
>>> On Wed, Dec 10, 2008 at 12:05 AM, Bill Lorensen <bill.lorensen at gmail.com>
>>> wrote:
>>>> Hans,
>>>>
>>>> The documentation for ShrinkImageFilter says that it does simple
>>>> subsampling. So it is not really a bug according to the documentation.
>>>> However, I think your implementation is more useful (I assume it is
>>>> correctly done). You should probably fix the documentation in the .h
>>>> file when you commit these changes.
>>>>
>>>> Also, beware that we have enabled a KWStyle commit check for some
>>>> directories. These directories are Testing/Code/Review, Code/Review,
>>>> Code/IO and Code/BasicFilters. All of these directories are currently
>>>> clear of any style defects.
>>>>
>>>> Bill
>>>>
>>>> On Tue, Dec 9, 2008 at 11:55 PM, Bill Lorensen <bill.lorensen at gmail.com>
>>>> wrote:
>>>>> Hans,
>>>>>
>>>>> Two of the tests are seg faulting.
>>>>>
>>>>> Bill
>>>>>
>>>>> On Tue, Dec 9, 2008 at 11:38 PM, Hans Johnson <hans-johnson at uiowa.edu>
>>>>> wrote:
>>>>>> Luis and Bill,
>>>>>>
>>>>>> I think I've uncovered a bug in the itkShrinkImageFilter that has
>>>>>> ramifications in any use of the
>>>>>> itkMultiResolutionPyramidImageFilterTest.
>>>>>>
>>>>>> I was writing a program that aligns the mid-sagital plane with the
>>>>>> center
>>>>>> of
>>>>>> an image, and found that as I went from low-resolution estimates to
>>>>>> high-resolution estimates, the center of mass of the object in the
>>>>>> different
>>>>>> scale images would change.
>>>>>>
>>>>>> A new test (currently failing) has been added, and the patch that
>>>>>> makes
>>>>>> that
>>>>>> test pass has been included in the Bug report:
>>>>>> http://public.kitware.com/Bug/view.php?id=8275
>>>>>>
>>>>>> This change, however, will cause any of the tests that depend on the
>>>>>> MultiResolutionPyramidImage (and prehaps others) to start failing.
>>>>>>  I'd
>>>>>> like
>>>>>> your advice on how to get this bug fix resolved in the least obtrusive
>>>>>> way.
>>>>>>
>>>>>> Thanks,
>>>>>> Hans
>>>>>>
>>>>>>
>>>>>
>>>>
>>
>>
>
> 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.