[Insight-developers] Re: PeriodicBoundaryCondition
Torsten Rohlfing
torsten at synapse.sri.com
Sat Feb 3 13:22:51 EST 2007
Luis, Bill:
The folowing occurred to me: if Luis' suggestion of inverting the data
flow between iterator and BC is implemented, then the test code should
be extended to include a case with an iterator of the PBC template type
analogous to my code that caused my original segfaults. In other words,
the test code should cover the (currently uncovered) case of an iterator
with genuine PBC instead of overridden CBC.
TR
Bill Lorensen wrote:
> Luis,
>
> Another idea. Perhaps the Override method should enforce the type of
> the BoundaryCondition that was used as the template parameter. I think
> the intent of the Override was not to provide a different BC, but
> rather to provide a different instance so that parameters could be
> set for the BC. If that is the case, the Override method could do the
> dynamic cast and throw an exception if the types do not match. In this
> case, Torsten's patch would be required.
>
> Bill
>
>
>
> On 2/2/07, *Luis Ibanez* <luis.ibanez at kitware.com
> <mailto:luis.ibanez at kitware.com>> wrote:
>
> Hi Torsten,
>
> Yes, it seems that there is a circularity problem in this design.
>
> In general, dynamic_casting should be avoided in generic programing,
> since, it introduces assumptions about the actual type of an object.
>
> Reviewing the code, it seems that the boundary condition uses the
> down casted iterator in order to gain access to the image pointer
> via the iterator->GetImagePointer() method.
>
> Instead of having the boundary condition request the image pointer
> this way, we could have a SetImage() method in the ImageBoundary
> Condition, and have the NeighborhoodIterator pass the image pointer
> to the boundary condition.
>
> In other words, the Neighborhood iterator could feed data to the
> boundary condition instead of the boundary condition having to query
> data from the iterator.
>
> The other methods that the PeriodicBoundary condition is invoking
> are already available in the Neighborhood and therefore no dynamic
> casting should be necessary.
>
> We are giving it a try to this modification. If it seems to work
> then we will send you a patch so you can give it a try.
>
>
> Thanks
>
>
> Luis
>
>
>
> =========================
> Torsten Rohlfing wrote:
> >
> > Hi Luis --
> >
> > I think I am starting to understand the issue, and it may well
> be a more
> > fundamental design problem. Here's what happens:
> >
> > In the test code, an iterator is declared as
> >
> > typedef itk::ConstNeighborhoodIterator<ImageType> IteratorType;
> >
> > with the default boundary condition template argument
> >
> > itk::ZeroFluxNeumannBoundaryCondition<ImageType>
> >
> > Then further down, the boundary condition is overridden with
> >
> > itk::PeriodicBoundaryCondition<ImageType> bc;
> > it.OverrideBoundaryCondition(&bc);
> >
> > Now, inside itk::PeriodicBoundaryCondition::operator(), the
> dynamic cast
> >
> > const ConstNeighborhoodIterator<TImage,Self> * iterator
> > = dynamic_cast<const ConstNeighborhoodIterator<TImage,Self>
> *>(data);
> >
> > of the iterator (pointed to by "data) returns NULL, because
> "Self" is
> > the periodic BC and not the default BC. This causes the segfault.
> >
> > The problem is that without the "Self" template parameter in the
> cast,
> > ie., without my patch, the same segfault happens when you originally
> > declare the iterator using the "correct" boundary condition.
> This is
> > what happened to me originally.
> >
> > So it seems that the problem is really with the fundamental
> > incompatibility of the iterator's "OverrideBoundaryCondition"
> method and
> > the dynamic cast inside the BC.
> >
> > Casting to the parent class if the iterator, ie.,
> "itk::Neighborhood"
> > doesn't solve the problem, because it doesn't provide the
> GetImagePtr()
> > member that is needed in
> itk::PeriodicBoundaryCondition::operator().
> >
> > While I won't claim to grasp the entire class design here, this
> seems to
> > suggest two possible fixes:
> >
> > a) Try casts to different template instantiations in
> > itk::PeriodicBoundaryCondition::operator() and use the one that
> gives a
> > non-NULL result. This isn't really practical, because we
> shouldn't hard
> > code all possible boundary conditions here.
> >
> > b) Find a different way to provide the iterator data to the
> boundary
> > condition, which doesn't involve RTTI casts.
> >
> > And then there's the workaround: always declare iterators with the
> > default BC and override it with the one we really want. This latter
> > options seems to require the least effort right now, but it should
> > probably be (better) documented if this is in fact required.
> >
> > I'll be very interested in hearing your thoughts!
> > Torsten
> >
> > Luis Ibanez wrote:
> >
> >> Hi Torsten,
> >>
> >>
> >> Thanks for reporting the problem with the
> >> PeriodicBoundaryCondition class:
> >>
> >> http://public.kitware.com/Bug/bug.php?op=show&bugid=4336&pos=3
> <http://public.kitware.com/Bug/bug.php?op=show&bugid=4336&pos=3>
> >>
> >> We just applied the patch that you kindly provided,
> >> but then the test for this class segfaults.
> >>
> >> Could you please verify if the Test is to be modified too ?
> >>
> >> Or maybe we should roll back this fix until there is more
> >> clarity on what the dynamic casting should use as type argument.
> >>
> >>
> >> Please let us know,
> >>
> >>
> >> Thanks
> >>
> >>
> >> Luis
> >>
> >>
> >
> >
> _______________________________________________
> Insight-developers mailing list
> Insight-developers at itk.org <mailto:Insight-developers at itk.org>
> http://www.itk.org/mailman/listinfo/insight-developers
> <http://www.itk.org/mailman/listinfo/insight-developers>
>
>
--
--
Torsten Rohlfing, PhD SRI International, Neuroscience Program
Research Scientist 333 Ravenswood Ave, Menlo Park, CA 94025
Phone: ++1 (650) 859-3379 Fax: ++1 (650) 859-2743
torsten at synapse.sri.com http://www.stanford.edu/~rohlfing/
"Though this be madness, yet there is a method in't"
More information about the Insight-developers
mailing list