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

Revision history for this message
Anders Logg (logg) 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? Or should it be a
general array class used throughout DOLFIN C++?

If it is just for Python-wrapping, I think we should make it never own
data as Joachim suggests.

--
Anders

> -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.
> >
>

« Back to merge proposal