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

kent williams norman-k-williams at uiowa.edu
Thu Apr 9 10:53:51 EDT 2009 

OK, once again I feel like there is still no consensus.

To summarize  the positions -- I think

1. Tom Vercauteren's latest proposal: Have an enum { NeverSmoothUnitShrink,
AlwaysSmoothUnitShrink, SmoothUnitShrinkIffScheduleNotBackwardsCompatible.
2. Leave filters as is/backwards compatible with Smoothing on for
MultiRes... and off for RecursiveMultiRes...
3. Hans' proposal, which I'm not sure I understand. I think it is #2 + added
documentation + SmoothUnitShrink flag.
4. The Luis Solution, which is #2 + adding new classes that explicity follow
#1, with the old classes having the existing name.

As I've said, I've been assigned the task of addressing this but I have no
dog in this fight.  I just need to know what to implement.

My only observation of the existing code is that the work I recently did
involved copying code out of MultRes... into RecursiveMultiRes... -- and
this means that the same or almost identical code is in two different
classes, one derived from the other. The common code needs to be put in
method/member variable combinations called from both places.

The Luis solution, which is probably the most Software-Engineering-Correct,
requires some careful refactoring to avoid further code-line replication.
And I don't know AT ALL what the new classes should be called, except that
the VTK 'append a 2 to the name' solution (e.g. vtkImageViewer2) is
annoying, and making a longer name out of
RecursiveMultiResolutionPyramidImageFilter isn't very attractive either.

On 4/9/09 8: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_SmoothIfU
>>>> nit
>>>> 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
>>> 
>> 
>> 
>> 
>>

More information about the Insight-developers mailing list