Comment 16 for bug 966553

On Wed, Mar 28, 2012 at 01:11:33PM -0000, Garth Wells wrote:

> > I don't see that as a big problem. The coordinates array is itself of
> > considerable size compared to the Mesh, and if someone calls
> > Mesh.coordinates() they will likely assume that the mesh is still
> > alive.
>
> I don't think so. From
>
> coord = UnitCube(1, 1, 1).coordinates()
>
> I would not expect the Mesh to be in scope.

Me neither, I would expect coord to be a copy.

But if I do

  mesh = UnitCube(1, 1, 1)
  coord = mesh.coordinates()

I expect to be able to modify the mesh coordinates by modifying coord.

> > If not, they could call Mesh.coordinates().copy().
> >
>
> We shouldn't make this assumption on what a user wants, and we
> shouldn't assume that the objects stored in a Mesh won't change. Say,
> for example, the Mesh data structure is changed in the future that it
> stores a hierarchy of refinements. Then by accessing the array of
> coordinates, one is, behind the scenes and unintuitively, keeping a
> possibly very hierarchy of objects in scope.
>
> We have had cases in past where we artificially keep objects in scope,
> and this has lead to unanticipated problems. This is why I believe
> that it's a flawed design that should be avoided.

I think this is something of a special case. The mesh.coordinates()
thing has been around for quite some time, it is very convenient and
I think it's being used quite a lot (I use it).

--
Anders

> Garth
>
> >
> >
> >> >>>>> I suggest that we
> >> >>>>>
> >> >>>>> 1. Have  one function that returns a copy of the coordinates and one
> >> >>>>> that provides view
> >> >>>>
> >> >>>> I tend to agree, but the question is what should be default. The most
> >> >>>> conservative would be to let a copy be default, but that would most
> >> >>>> probably break user code, as most people (hmpfr me) uses coordinates to
> >> >>>> move meshes.
> >> >>>
> >> >>> I think we should try hard to avoid that. Accessing coordinates() by
> >> >>> reference (not copy) has been the default behavior and we would break
> >> >>> user code, possibly also some code examples from the book.
> >> >>>
> >> >>>>> or
> >> >>>>>
> >> >>>>> 2. Make the coordinate data a boost::shared_array<double>     or a
> >> >>>>> boost::shared_ptr<std::vector<double>     >     object.
> >> >>>>
> >> >>>> This would introduce a somewhat middle solution, which is pretty
> >> >>>> cumbersome to implement and which need to be changed anyhow, when we
> >> >>>> revamp the mesh interface.
> >> >>>
> >> >
> >> > It should be easy to implement - I have already changed some plain
> >> > arrays to std::vector. Relying on the contiguous storage that a
> >> > std::vector provides, it's easy to interface with functions looking to
> >> > accept a plain pointer. The question is how it should work in the SWIG
> >> > layer.
> >>
> >> Exactly, and here NumPy provides an excellent functionality. For example
> >> if we a provide a shared_ptr version of std::vector we might be able to
> >> do things in the way you suggest, but then we are out of NumPy land.
> >>
> >> A middle way might be to create a dummy Python object which holds a
> >> reference to a shared_ptr vector, which we then created a NumPy array
> >> around. This dummy object could then be attached to the NumPy array
> >> preventing the data to go out of scope, but I do not see how that would
> >> help us.
> >>
> >> Johan
> >>
> >> >>> Is it up for revamping???
> >> >>
> >> >> :)
> >> >>
> >> >> You keep on talking about this. How on earth are we going to support
> >> >> coarsening, refinement, and parallel such if we do not make the storage
> >> >> of coordinates, and connectivities more flexible. And if that will be
> >> >> the case, providing a numpy array view to these entities need to be
> >> >> rethinked.
> >> >>
> >> >
> >> > Yes, but it does require some thinking.
> >> >
> >> > Garth
> >> >
> >> >> Johan
> >> >>
> >> >>>
> >> >>>
> >> >>>> I vote for adding a reference to the returned object, which happens to
> >> >>>> be the solution I just implemented and are now testing ;)
> >> >>>
> >> >>>
> >> >>>> Johan
> >> >>>>
> >> >>>>>
> >> >>>>> Garth
> >> >>>>>
> >> >>>>>> ** Changed in: dolfin
> >> >>>>>>          Status: Confirmed =>     In Progress
> >> >>>>>>
> >> >>>>>>
> >> >>>>>> Title:
> >> >>>>>>     BoundaryMesh.coordinates () gives random results
> >> >>>>>>
> >> >>>>>> To manage notifications about this bug go to:
> >> >>>>>> https://bugs.launchpad.net/dolfin/+bug/966553/+subscriptions
> >> >>>>>
> >> >>>>
> >> >>>
> >> >>
> >> >>
> >> >> Title:
> >> >>   BoundaryMesh.coordinates () gives random results
> >> >>
> >> >> To manage notifications about this bug go to:
> >> >> https://bugs.launchpad.net/dolfin/+bug/966553/+subscriptions
> >> >
> >>
> >
> >
> > Title:
> >  BoundaryMesh.coordinates () gives random results
> >
> > To manage notifications about this bug go to:
> > https://bugs.launchpad.net/dolfin/+bug/966553/+subscriptions
>