[Insight-developers] So -- no concensus on smoothing re MultiResolutionPyramidImageFilter?
Luis Ibanez
luis.ibanez at kitware.com
Thu Apr 9 16:41:34 EDT 2009
Kent,
Here is a suggestion for the names of the new classes:
A) itkMultiResolutionPyramidImageFilterBase
(move here all the code of the common API:
(setting schedules, Set input images, get
output image... etc)
B) Keep the itkMultiResolutionPyramidImageFilter
class, and remove from it any code that goes
into the base class in (A).
[mark it as deprecated]
C) Keep the itkRecursiveMultiResolutionPyramid on
and also remove from it any code that went into
the base class in A.
[mark it as deprecated]
D) add a new class
itkMultiResolutionNearestNeighbordInterpolatedPyramid....
that will use the Shrink filter in order to
provide short computation time.
E) add a new class
itkMultiResolutionLinearInterpolatedPyramid...
that will use the ResampleImageFilter and the
Linear Interpolator, and will do its best for
maintaining the center of mass location for
the images across all the levels.
F) another one ?
And... it may or may not be worth having (B) and (C)
deriving from the new MultiResolutionPyramid filter.
It all depends on how similar their global API end up
being.
An alternative design could be to follow the architecture
of the ResampleImageFilter, and to have the new MultiResolution
ImageFilter to be a class that collaborate with helper classes
that the user will connect to it. That design will be closer
to what the ImageMetrics do.
The safest way to go about this will be to write these
classes in a subdirectory of the NAMIC Sandbox, so we
can have the design and code discussions over there and
we can experiment with the code until we reach concensus
and satisfaction.
Then post it to the Insight Journal, and then finally
move it into ITK.
It may look like a longer path, but at least,
we will be moving in a single and consistent direction....
Luis
---------------------
kent williams wrote:
> 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