[Insight-developers] So -- no concensus on smoothing re MultiResolutionPyramidImageFilter?

Thu Apr 9 11:03:53 EDT 2009

Nice!
s

On Thu, Apr 9, 2009 at 9:28 AM, Luis Ibanez <luis.ibanez at kitware.com> 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
>>>
>>
>>
>>
>>
>

-- 
Stephen R. Aylward, Ph.D.
Director of Medical Imaging
Kitware, Inc. - North Carolina Office
http://www.kitware.com
(518) 371-3971 x300