[Insight-developers] pyramid class
Simon Warfield
simon.warfield at childrens.harvard.edu
Thu Apr 9 10:51:57 EDT 2009
I agree too.
The current filter has a behavior some groups are depending on, but it
has many undesirable characteristics.
Rather than try to create a multi-option filter that is all things to
all people, I think it would be better to restore the filter to how it
was prior to 3.12,
clearly mark it as something no new code should use, and provide a
replacement that :
1. can maintain the physical coordinate system across levels of the pyramid,
2. optionally uses an appropriate low pass filter to achieve smoothing,
3. uses an appropriate resample filter rather than internally
re-implementing resampling,
4. has a set of test cases that demonstrate the correct behavior.
So that just leaves fixing the resampler filters to not require special
versions for vectors or to dereference out-of-bounds locations,
fixing the transform hierarchy (including enabling appropriate inverse
transforms), and
defining and implementing the physical coordinate system consistently.
insight-developers-request at itk.org wrote:
> Date: Thu, 9 Apr 2009 09:34:58 -0400
> From: Bradley Lowekamp <blowekamp at mail.nih.gov>
> Subject: Re: [Insight-developers] So -- no concensus on smoothing re
> MultiResolutionPyramidImageFilter?
> To: Luis Ibanez <luis.ibanez at kitware.com>
> Cc: "tom.vercauteren at m4x.org" <tom.vercauteren at m4x.org>, Hans Johnson
> <hans-johnson at uiowa.edu>, Stephen Aylward
> <stephen.aylward at kitware.com>, ITK <insight-developers at itk.org>, Kent
> Williams <norman-k-williams at uiowa.edu>
> Message-ID: <2184EB5C-ACDC-4410-B818-76B451228D40 at mail.nih.gov>
> Content-Type: text/plain; charset="us-ascii"; Format="flowed";
> DelSp="yes"
>
> I agree.
>
>
> On Apr 9, 2009, at 9:28 AM, Luis Ibanez wrote:
>
>
>> This filter is reaching the point
>> where it is not recoverable.
>>
>>
>> This discussion start looking like a symposium on how
>> to fix a duck-tape patch with another duck-tape patch.
>>
>>
>> I vote for :
>>
>>
>> a) Leaving it in a state that is closer to what we
>> could call backward compatible.
>>
>> b) Then mark it as deprecated,
>> and discourage anybody from using it again.
>>
>> c) Write a new filter(s), with a new name(s).
>> From a fresh start based on the requirements
>> that we know today.
>>
>>
>> The requirements could be:
>>
>> 1) Performance when no smoothing is needed.
>>
>> 2) Keeping the center of mass of the images at different levels.
>>
>> 3) ...more.. ?
>>
>>
>> If the requirements turn out to be conflictive, then I would vote
>> for writing separate variations of the filter, and make each one
>> of them fine tunned for its correct use.
>>
>> Saturating a class with booleans and enums is a symptom that we
>> are failing to design a hierarchy of classes where the various
>> behaviors should be implemented in separate derived classes.
>>
>> Using the "boolean" and "enum" approach, one would have to wonder
>> why is that we don't have a single monstrous itkOptimizer class,
>> and through dark-magic combinations of boolean settings we make it
>> behave like a : GradientDescent, or Powell, or Evolutionary....,
>> or why don't we have a single itkImageMetric class, and through
>> enums settings we make it behave as Mutual Information, or
>> MeanSquares,
>> or NormalizedCorrelations...
>>
>> There is no reason why we have to stick to a single
>> "MultiResolutionPyramidImageFilter" when we could have a clean
>> framework/hierarchy with multiple classes, where each one:
>>
>>
>> "do one thing, do it well"
>>
>>
>>
>> Luis
>>
>> --
>>
>>
>> PS.
>>
>> High density of "if" statements (and its cousin, the "switch")
>> is an indicator of poor software design, and/or poor software
>> maintenance practices. We can do much better than that...
>>
>>
>> ----------------------
>> Stephen Aylward wrote:
>>
>>> I could be misunderstanding...but shouldn't the default be "true" to
>>> keep backward compatibility?
>>>
>>> Having an option to offer backward compatibility, but having the
>>> default be to not enable backward compatibility seems very odd (ok,
>>> it
>>> seems simply wrong).
>>>
>>> Bill?
>>>
>>> Luis?
>>>
>>> Stephen
>>>
>>> On Thu, Apr 9, 2009 at 7:30 AM, Hans Johnson <hans-
>>> johnson at uiowa.edu> wrote:
>>>
>>>
>>>> Since this is considered a bug for over a year
>>>> http://www.vtk.org/Bug/view.php?id=7002 we should consider the
>>>> fact that
>>>> MultiResolutionPyramidImageFilter smooths for unit shrink factors
>>>> a bug that
>>>> would allow us to fix it (even through numerical sameness/backwards
>>>> compatibility will be lost).
>>>>
>>>> This is not the behavior that I would expect by reading the
>>>> documentation on
>>>> how an Image Pyramid should be constructed (i.e. The top level
>>>> should be an
>>>> exact copy of the reference images).
>>>>
>>>> I think that this is a bug that also affects the
>>>> RecursiveMultiResolutionPyramidImageFilter because it would produce
>>>> different results for the unit shrink factor image if I change the
>>>> shrink
>>>> schedule from being downward divisible to being non-downward
>>>> divisible.
>>>> This is particularly nasty because given the same "shrink schedule
>>>> [8,4,2,1]" the unit shrink result will be different depending on
>>>> the size of
>>>> the input image.
>>>>
>>>> ===============
>>>> If there are no objects today, then the solution that will be
>>>> implemented
>>>> will be:
>>>>
>>>> 1) Add a test that fails when the unit shrink image is not the
>>>> same as the
>>>> reference image (and the m_SmoothIfUnitShrinkFactors =false).
>>>> 2) Add the m_SmoothIfUnitShrinkFactors conditional flag to allow
>>>> explicit
>>>> smoothing of the unit shrink factors.
>>>> 3) Set the default to "false" in all cases.
>>>> ===============
>>>>
>>>>
>>>> Regards,
>>>> Hans
>>>>
>>>>
>>>> On 4/9/09 1:56 AM, "Tom Vercauteren" <tom.vercauteren at gmail.com>
>>>> wrote:
>>>>
>>>>
>>>>
>>>>> Hi Kent,
>>>>>
>>>>> Even if there is no consensus about what the default should be, it
>>>>> could be add a flag that lets the user decide (as Stephen
>>>>> suggested).
>>>>>
>>>>> Maybe something like:
>>>>> m_SmoothIfUnitShrinkFactors
>>>>>
>>>>> For backwards compatibility, the default for this flag would be:
>>>>> - true for MultiResolutionPyramidImageFilter
>>>>> - false for RecursiveMultiResolutionPyramidImageFilter
>>>>>
>>>>> Now, I also would like to propose a NON-backward compatible
>>>>> change to
>>>>> fix something that might be very confusing to the user.
>>>>>
>>>>> Since RecursiveMultiResolutionPyramidImageFilter wil call
>>>>> MultiResolutionPyramidImageFilter if the schedule is not downward
>>>>> divisible, I suggest that
>>>>> RecursiveMultiResolutionPyramidImageFilter
>>>>> sets
>>>>>
>>>>> internalNonRecursivePyramid->SetSmoothIfUnitShrinkFactors(this-
>>>>>
>>>>>> m_SmoothIfUnit
>>>>>>
>>>>> ShrinkFactors)
>>>>> when the schedule is not downward divisible.
>>>>>
>>>>> With this small non-backward compatible change, it would at least
>>>>> make
>>>>> things a bit clearer.
>>>>>
>>>>> Otherwise we might need a three state flag for
>>>>> RecursiveMultiResolutionPyramidImageFilter:
>>>>> - AlwaysSmoothIfUnitShrinkFactors
>>>>> - NeverSmoothIfUnitShrinkFactors
>>>>> - SmoothIfUnitShrinkFactorsAndScheduleDownardDivisible (this being
>>>>> the default for true backwards compatibility)
>>>>>
>>>>> Thoughts anyone?
>>>>>
>>>>> Tom
>>>>>
>>>>> On Wed, Apr 8, 2009 at 23:27, kent williams <norman-k-williams at uiowa.edu
>>>>>
>>>>> wrote:
>>>>>
>>>>>
>>>>>> It seems Hans assigned me yesterday to fix this 'bug':
>>>>>>
>>>>>> http://www.vtk.org/Bug/view.php?id=7002
>>>>>>
>>>>>> As is noted there, there still doesn't appear to be a consensus
>>>>>> on how to
>>>>>> handle this problem. So I've taken ownership of a bug with no
>>>>>> clear way to
>>>>>> fix it.
>>>>>>
>>>>>> If the issue is truly tabled, does it need to hang around as an
>>>>>> open bug?
>>>>>>
>>>>>>
>>>>>>
>>>>>> 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
>>>>>>
>>>>>> 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
>>>>>
>>>> _______________________________________________
>>>> 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
>>
>
> ========================================================
> Bradley Lowekamp
> Lockheed Martin Contractor for
> Office of High Performance Computing and Communications
> National Library of Medicine
> blowekamp at mail.nih.gov
>
>
>
--
Simon
More information about the Insight-developers
mailing list