Re: [ublas] Array adaptor changes

From: Michael Stevens (mail_at_[hidden])
Date: 2005-08-27 14:20:04

Hi Vardan,
On Thursday 25 August 2005 20:05, Vardan Akopian wrote:

> I was (and still am) heavily using the shallow_array_adaptor class. I
> have not upgraded yet to 1.33.0 and haven't gotten a cvs update for
> about 2 months, so I can't report any factual problems, but these
> changes worry me. Comments inline.

Thanks for your analysis. As a shallow_array_adaptor user your insight is very 
helpful.

> On 8/20/05, Michael Stevens <mail_at_[hidden]> wrote:

> >  b) In order to make them Copy Construtable they would allocate their own
> > storage.
>
> This is not true for shallow_array_adaptor (it's true for
> array_adaptor). The shallow_array_adaptor would just copy the internal
> data_ member, which is a boost::shared_array.

Good point. 'shallow_array_adaptor' only allocated its own storage in the 
sized constructor.

> >         ii) It is very easy to accidentally perform operations which
> > cause construction and then expected changes to the adapted array would
> > not happen.
>
> Not true for the shallow_array_adaptor for the above reasons.
Agreed.

> > In new version the two existing classes have been replaces by the simpler
> > 'carray_adaptor'. This is now fully documented. The changes are committed
> > to Boost HEAD so please take a look at the documentation.
>
> Thank you very much for the documentation.
> I took a look at the new class, and it looks like you enforce that
> carray_adaptor is not copy constructable.

Correct.

> Doesn't this effectively 
> render it useless as an adaptor for any matrix class?

It only restricts its usage!  This is fundamental, if we restrict our selves 
to only using the storage (adapted arrays) specified the all the Storage 
concept requirements cannot be met. Containers using such storage classes 
cannot be used where temporises or copies are required.

'carray_adaptor' is designed to be the minimal solution when adapting. It has 
minimal overhead but requires you restrict your usage.

> Many (all?) 
> matrix classes copy construct their internal array when they are copy
> constructed
> (or is this changed in 1.33.0 too?). 
Still the same!

> I also don't see any reason not to allow copy construction for
> carray_adaptor. Why not just copy the pointer (as
> shallow_array_adaptor was in effect doing)?

Copying the pointer is one solution. I think this is better supported by 
shallow_array_adaptor however. It important to note that shallow copy also 
breaks the semantics for Storage.

Code written for default matrix may produce very different results when used 
with a matrix with shallow copy. Containers which automatically shallow copy 
usually result in nasty surprises when you accidentally create an alias. I 
think we need both array adaptors that make this kind of ambiguous usage 
invalid and those that allow it for the brave!

I think at least the basic array adaptor should avoid shallow copies. This 
fits well with 'carray_adaptor' because it is designed to provide a minimal 
efficient implementation.

> And to make it consistent 
> with this (and for another reason below), I think the assignment
> operator should behave the same way, i.e. copy the pointer, instead of
> resizing and copying the data as it does right now.
> The way it's written, I don't think carray_adaptor would even compile,
> since it's calling resize_internal(), but it has no such member
> function. Am I missing something?

I have fixed (commited to HEAD) resize now. Thanks for pointing this out. The 
correct 'resize' in 'carray_adaptor' works much as that in bounded_array. It 
simply changes how many elements are used from the already allocated storage.

> Isn't the idea of carray_adaptor (and shallow_array_adaptor) to use an
> externally allocated memory for storage without doing any internal
> allocation? Doesn't this mean that it is not (and should not) be
> responsible for deallocating the storage?

Agreed. It is interesting to note that 'shallow_array_adaptor' only needs to 
keep track of storage in the case where a sized constructor was used and it 
created its own storage. We could create a much more efficient 
'shallow_array_adaptor' if we disallowed the sized constructors.

We would then end up with two array adaptor which are efficient. One which 
cannot be copied and one which can be shallow copied. Both could not be sized 
constructed and therefore vectors/matrices expressions using them would have 
to avoid temporises. When writing code that uses adapted arrays I think the 
latter restriction is the right way to go.

> In conclusion, here is how I would change the current carray_adaptor
> 1) allow copy constructor by just copying the internal pointeinr to the
> storage.

> 2) remove all the resize functions (or declare then as private and 
> not implemented, or use whatever technique boost has for this type of
> control). carray_adaptor by its nature should not be resizable.
See corrected code.

> 3) change the assignment operator to only copy the pointer to the
> storage (and the size) instead of copying the data.

This could be dangerous. If we did this then
A=B; would cause a shallow copy
while
A= <anything else>; would copy elements.

> 4) add test cases for carray_adaptor (are there any already?), since
> it's now an officially documented class ;-).

Test cases are are real pain. Because the array adaptors don't meet the full 
requirements for the Storage model we cannot reuse the normal tests.

Thanks again. Hopefully we can work out way to a fully documented, logical and 
efficient set of adaptors!

Michael

-- 
___________________________________
Michael Stevens Systems Engineering
34128 Kassel, Germany
Phone/Fax: [phone]
Navigation Systems, Estimation  and
                 Bayesian Filtering
    http://bayesclasses.sf.net
___________________________________