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

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.

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

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?

Garth

> --
> 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.
>> >
>>
>
> --
> https://code.launchpad.net/~jobh/dolfin/fast-array/+merge/94467
> Your team DOLFIN Core Team is requested to review the proposed merge of lp:~jobh/dolfin/fast-array into lp:dolfin.