dolfin team mailing list archive

[Anders Logg <logg@xxxxxxxxx>]

On Thu, Feb 10, 2011 at 09:16:20AM +0000, Garth N. Wells wrote:
>
>
> On 10/02/11 09:13, Anders Logg wrote:
> > On Thu, Feb 10, 2011 at 08:44:21AM +0000, Garth N. Wells wrote:
> >>
> >>
> >> On 10/02/11 07:19, Anders Logg wrote:
> >>> B1;2600;0cOn Wed, Feb 09, 2011 at 11:03:28PM +0000, Garth N. Wells wrote:
> >>>>
> >>>>
> >>>> On 09/02/11 22:43, Anders Logg wrote:
> >>>>> On Wed, Feb 09, 2011 at 10:11:40AM -0800, Johan Hake wrote:
> >>>>>> Hello!
> >>>>>>
> >>>>>> I am pretty sure the reason the Macbot still complains (mesh unit test) is
> >>>>>> that refine is broken for SWIG 2.0.
> >>>>>>
> >>>>>> I think it is some premature destruction of a refined mesh. I would suggest we
> >>>>>> implement a full shared_ptr version of the interface to get around this
> >>>>>> problem. I have no clue of why it works for SWIG 1.3.40. Probably because a
> >>>>>> faulty implementation.
> >>>>>>
> >>>>>> I also suggest more developers upgrade to SWIG 2.0.1 and maybe one of the
> >>>>>> linux build bots two? If it is only the Macbot that uses SWIG 2.0 it is easily
> >>>>>> to think it is some Mac specific error.
> >>>>>>
> >>>>>> Johan
> >>>>>
> >>>>> I plan to merge with main tomorrow if my buildbot is green. Then Marie
> >>>>> also needs to merge (we have both touched refine.h/cpp). Then we can
> >>>>> sort out the shared_ptrs.
> >>>>>
> >>>>
> >>>> I really don't get the approach to hierarchies. Mesh refinement was
> >>>> simple, and now something simple has become complex (with bugs that are
> >>>> hard to track down because of the introduced complexity).
> >>>
> >>> I really don't understand what the problem is. Mesh refinement is just
> >>> as simple as it used to be, the Mesh class itself is (almost) as
> >>> simple as it used to be (one added inheritance), and the Hierarchical
> >>> base class is also a very simple class (just stores two pointers for
> >>> child and parent). All the complexity is in refine.cpp, which only
> >>> kicks in if you do something like refine(variational_problem) or
> >>> refine(function_space).
> >>>
> >>>> This hierarchy business looks viral - now I get Swig warnings for
> >>>> DirichleBC.
> >>>>
> >>>> I would much rather keep basic classes simple, and have hierarchical
> >>>> containers that can keep track of parent/child relationships. The
> >>>> present approach seems to take a narrow/immediate view on the issue.
> >>>
> >>> I think that would lead to more complex code. We would need to store
> >>> quite a few different relationship trees separately from the objects.
> >>>
> >>> The problems we see now are related to SWIG and correct use of
> >>> shared_ptrs. We've had quite a few SWIG/shared_ptr problems in the
> >>> past and figured out how to fix them. I'm sure we will handle this
> >>> too.
> >>>
> >>
> >> I would like to remove the mesh refine functions, or re-write them. My
> >> objections to the current implementation are:
> >>
> >>  - It forces a mesh hierarchy on users, even if this is not desired
> >
> > It doesn't. Normal users will do
> >
> >   mesh = refine(mesh);
> >
> > and then no hierarchy is stored (in C++). It would be very easy to add
> > a parameter "store_hierarchy" which defaults to false if that is
> > desired.
> >
> >>  - It is not const-correct.
> >
> > I believe it is const-correct in Marie's branch.
> >
> >>  - The 'parent' can go out of scope, leaving the 'child' with a dangling
> >> pointer.
> >
> > It shouldn't go out of scope since we use shared ptrs. It's the same
> > issue as everywhere else where we pass by reference and use
> > reference_to_no_delete_pointer. It's not special for the refinement
> > functions.
> >
>
> It is different, because elsewhere we provide an 'advanced' interface
> that takes shared pointers.

That's an easy fix. I'll wait for the merge with Marie's branch and
add it then.

I really don't see what the big issue is. If there are some details of
the implementation which are not 100% correct (like missing duplicate
versions taking references or shared_ptr) we will fix those details.
It's no reason for throwing out the implementation and starting from
scratch. The implementation itself is in pretty good shape and solves
a complex problem in a relatively small amount of code.

--
Anders