[Insight-developers] So -- no concensus on smoothing re MultiResolutionPyramidImageFilter?
kent williams
norman-k-williams at uiowa.edu
Fri Apr 10 12:03:34 EDT 2009
I dont generally have the NAMIC sandbox checked out. What are the mechanics
of adding directories to it?
And maybe getting the cart before the horse, but how do you put a class in
Insight/Code/Review that will replace a class already in Insight/Code
elsewhere?
And again, there seem to be various issues that Luis is conflating.
1. Are you proposing to remove the UseShrinkImageFilter method from
MultiResolutionPyramidImageFilter? I thought that was a settled solution,
with using ResampleImageFilter as the default.
2. The ShrinkImageFilter now does do a good job of maintaining image
centers. Good enough that the tests pass the regressiontest code Hans added
to verify this. The test succeeds with some variance from the expected
values, but within the epsilon Hans put in the test. The ResampleImageFilter
is better -- enough better to be preferable in applications dependent on
minimal center creep.
3. I thought the BIG controversy left was whether or not to smooth the
output for a schedule of 1. That was when Luis suggested having a new class
to implement the no-smoothing-with-schedule-of-one version of
MultiResolutionPyramid...
I think that the proliferation of classes with slightly differening
behaviors is no less a problem than proliferation of configuration methods.
Sometimes using one method to move ahead is appropriate, sometimes the
other.
Someone said "The fact we have been spending more time in discussion how a
recent patch will not break the patch of a previous patch, rather than
spending time adding new algorithms to the toolkit...is a bad sign"
I disagree with that. This is exactly the sort of discussion that is crucial
to keeping ITK moving forward properly. This is not the sort of discussion
that occurs around most other software packages, to their detriment.
Consider Microsoft's development and maintainence of their own APIs -- they
come out of a closed source, closed culture, and developers just have to
cope with whatever Microsoft rolls down the mountain at them. They change
APIs, orphan tools, provide hundreds of obscure ill-documented features that
developers have to learn to use by trial and error.
We're at least taking the process seriously, even if it doesn't always seem
to run smoothly.
On 4/9/09 3:41 PM, "Luis Ibanez" <luis.ibanez at kitware.com> wrote:
>
>
> 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_SmoothI
>>>>>> fU
>>>>>> 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