Code review comment for lp:~jobh/dolfin/fast-array

On Mon, Feb 27, 2012 at 03:32:04PM -0000, Johan Hake wrote:
> On 02/27/2012 03:01 PM, Garth Wells wrote:
> > On 27 February 2012 13:40, Anders Logg<email address hidden> wrote:
> >> On Fri, Feb 24, 2012 at 07:01:57PM -0000, Joachim Haga wrote:
> >>> I'd opine that the easiest would be to make array never own the data,
> >>> rather than always own the data. In that way, it can wrap anything.
> >>>
> >>> * TimeSeries returns (owned) Arrays in two places. These could
> >>> return references to its own data (const vector<double>&), that
> >>> would save a copy (they are currently copied twice, once into an
> >>> owned Array, then again in swig into a numpy array). But maybe the
> >>> amount of data returned is small, in which case a vector<double>
> >>> will be fine.
> >>
> >> The return of arrays from TimeSeries is typically not performance
> >> critical so a copy is fine (which it already is).
> >>
> >>> * Can't support resize() without ownership, so everywhere resize is
> >>> used on a reference argument should pass a vector<double>&
> >>> instead. I think this is a few places only.
> >>>
> >>> At that point, Array will be a plain wrapper of others' data.
> >>>
> >>> As for your (1), it might well be possible (keep a scratch array in
> >>> the GenericFunction class perhaps). But the thing is, the shared
> >>> version was no more safe. You can't keep a reference to an Array
> >>> because you don't know if it's shared or just wrapping data that
> >>> will be deleted from under you. It was "shared or borrowed" rather
> >>> than "owned or borrowed", only without the flag to tell the cases
> >>> apart.
> >>
> >> I'm not very happy with our current use of Array. We use std::vector
> >> in some places and Array in other places. I don't remember what the
> >> original intention was with Array. Is the purpose just to be able to
> >> pass shared data as arrays back and forth to Python?
> >
> > Yes.
> >
> >> Or should it be a
> >> general array class used throughout DOLFIN C++?
> >>
> >
> > No - there is no point in re-inventing std::vector. We should use
> > std::vector where possible. It's more flexible and can be used with
> > STL algorithms.
>
> Agree.
>
> >> If it is just for Python-wrapping, I think we should make it never own
> >> data as Joachim suggests.
>
> Agree. But we need to go over the interface and check that it makes
> sense everywhere. TimeSeries could probably just return a
> std::vector which is copied in the Python interface.

Yes, definitely for TimeSeries. Perhaps we could settle for that
everytime we want to pass output data to Python?

Then Array input would essentially be limited to a few particular
callbacks that need to be made efficient (GenericFunction::eval).

--
Anders

> > Memory has to be created somewhere in order to be used, and it has
to
> > be cleaned up.
> >
> > There are two issues here:
> >
> > 1) Dynamic allocation in order to interface with UFC. Using plain
> > pointers rather than smart pointers is likely faster, but I think that
> > it's still not a solution. We should try to avoid these allocations in
> > loops.
> >
> > 2) We need to re-visit the NumPy interface. If we can create NumPy
> > arrays of fixed length, then we can just use std::vector, and let the
> > NumPy array wrap the pointer&x[0]. Maybe Johan Hake can comment on
> > this?
>
> The problem is to pass a NumPy array to the C++ interface when it wants
> a std::vector&. Then we need to copy the values. A light weight Array
> class fixes that issue.
>
> Wrapping a std::vector& into a Numpy array is possible without copying
> by just passing the pointer. This would be similar to what we do today
> with the Array.
>
> Johan
>
> > Garth
> >
> >>
> >>
> >>> -j.
> >>>
> >>> On 24 February 2012 19:37, Garth Wells<email address hidden> wrote:
> >>>
> >>>> I'm not keen on ownership flags - they were a real mess before and it's
> >>>> been a lot better since we go rid of them.
> >>>>
> >>>> Would be it be possible to (1) re-use the Array so we avoid dynamic
> >>>> allocation or (2) have an Array-like structure that always owns the data?
> >>>> The class Array was introduced to better interface with Python, but I
> >>>> recall that we later became aware that the Numpy interface has has a flag
> >>>> to prevent resizing, which would make typemaps more robust.
> >>>>
> >>>
> >>
> >
>
>