Re: [multi] Formal Review Begins

On 3/9/26 20:30, Alfredo Correa via Boost wrote:
>> The proposed library uses an array-of-arrays model.
> 
> 
> Multi supports both models array-of-array and flattened view this is
> prominently mentioned in the documentation, in the Iteration section (
> https://correaa.github.io/boost-multi/multi/tutorial.html#tutorial_iteration
> )

My bad.  I wrote my first message before fully reading the 
documentation, so I (somehow) missed the elements function.

> for(auto indices : a.extensions().elements() ) {
>       apply(a, indices) = get<0>(indices) + 10*get<1>(indices);
> }

I don't see subarray::extensions()::elements in the documentation at 
all.  In fact, the reference documentation for subarray::extensions() 
read "returns a tuple with the extensions in each dimension", and 
std::tuple does not have a elements member function.  I see one 
reference to multi::extensions_t and two references to 
subarray::extensions_type, neither of which mentions an elements member 
function.  So this one isn't my fault.

I didn't think of using std::apply here.  It's a clever solution that 
only works because arrays can use operator() for indexing instead of the 
more obvious operator[].  That said, I would still prefer the direct 
syntax "a[indeces]".

>> The use of "rotated" to switch the order of dimensions is unfortunate
>> because it conflicts with the meaning of rotating an image, i.e. turning
>> this:
>>      {{1, 2, 3},
>>       {4, 5, 6},
>>       {7, 8, 9}}
>> ...into this:
>>      {{7, 4, 1},
>>       {8, 5, 2},
>>       {9, 6, 3}}
>> Instead, it turns it into this:
>>      {{1, 4, 7},
>>       {2, 5, 8},
>>       {3, 6, 9}}
>> ...which, geometrically seen, is flipping operation over the diagonal,
>> not a rotation.
>>
> 
> The name is based on the "rotation of indices",
> a(i, j, k) turns into a(j, k, i), etc.
> 
> Take into account that the names must apply in the multidimensional
> context. There is a collision of interpretation in the 2D case here.

The geometric meaning of "rotate" is not limited to 2D.  I can rotate an 
object in 3D, although I have a lot more choices for the axis of rotation.

>> The boost::multi::array::reextent function is not clearly defined.
> 
> You will obtain {{1, 2, 0}}, a default element will appear at a[0][2]
> because a[0][2] was not part of the original array.
> 
> Your second option depends on some interpretation of the array as flat
> array of numbers.

I agree that this is the correct behavior, but the documentations should 
be more explicit.

>> The constructors of boost::multi::dynamic_array and boost::multi::array,
>> as well as the function boost::multi::array::reextent, require copy
>> construction.  It would be more flexible to allow emplacement from any
>> set of variables that can be used to construct an element, similar to
>> std::vector::emplace_back.
>>
> 
> I thought about this, but it is a false optimization.
> Basically the idea is that if you want to generate nxm copies of an
> elements, it is better to first make a master copy at the call site and the
> copy this elements. That is how std::vector constructor works. Optimizing
> for the 1-element arrays is not useful in the context.
> This is fundamentally different from ::emplace_back.

I'm thinking less about performance and more about storing non-copyable 
types in a multidimensional array.  The requirements of the element type 
are never documented, so I'm thinking it should work on basically all 
types., including non-copyable and non-movable types.

> But you can do this better with restrictions, and second, variadic
> templates in a constructor can mess up think in a nasty way.
> multi::array({5, 2}, arg1, arg2, arg3);  (to construct elements T{arg1,
> arg2, arg3}).

Using a restriction eliminates the need for copy construction, but it 
still requires move construction.  I admit that storing non-movable 
types in a multidimensional array is very niche.

> Having said that I can add tagged constructor, `multi::array(std::in_place,
> {5, 2}, arg1, arg2, arg3);` if you think it is something that you need.

That would solve the problem.

>> On a technical level, the web page doesn't accept arrow keys or page
>> up/down for scrolling, so I have to rely use my mouse for scrolling.
>>
> 
> I am able to navigate, scroll up and down with arrow keys, page up/down,
> and space.
> I am using a state of the art (I am told), documentation format (Antora).

All I can say is that it doesn't work for me on Firefox, my default 
browser.  It does work on Chromium, so this could be a browser 
incompatibility issue or a conflict with one of my browser extensions.

>> Also on a technical level, the right side of some of the tables is cut
>> off on my web browser even though there is blank space to the right of
>> the table.  I am forced to rely on horizontal scrolling to see the whole
>> table, and there isn't even a horizontal scroll bar.
>>
> 
> Except in the reference section, I have no problems rendering the tables on
> web browsers, all fit nicely in my browser window (Safari on Mac, Firefox
> on Linux).
> Can you send a screenshot of what you see so Matt Borland and other from
> Boost can help me make them fit in your window.

>> The boost/multi/elementwise.hpp header is not documented at all in the
>> reference section.
>>
> 
> It is documented here:
> https://correaa.github.io/boost-multi/multi/tutorial.html#tutorial_elementwise

Yes, but there's no list of functions/operators that the header 
includes.  Scrolling through the section, I see operators +, *, and /, 
as well as function "exp".  What other operators are there?  I assume 
that binary and unary - exist and unary + doesn't, but what about 
bitwise operators?  Any functions besides "exp"?

>> The documentation for (I assume)
>> boost::multi::subarray::const_element_ref just calls it element_ref.
>>
> 
> For restrictions? not necessarily, depends on the actual element returned,
> but what is your question?

On the page, 
https://correaa.github.io/boost-multi/multi/reference.html#ref_subarray, 
second table, there are two entries for "element_ref".  One reads 
"Reference to element type, deduced from pointer type, typically T&", 
the other reads "Constant reference to element type, deduced from 
pointer type, typically T const&".  This is an error in the 
documentation.  The second entry should be labeled "const_element_ref", 
not "element_ref".

>> boost::multi::subarray::index_range and
>> boost::multi::subarray::extension_type are only described in very vague
>> terms.  What are the members of these types?
> 
> index_range is whatever type you put in the parenthesis notation to
> indicate a range of elements
> 
> A( {2, 4} , 5 );  // is the array {A(2 , 5 ), A(3, 5} }
> 
> {2, 4} is converted to index_range.

Sure, but can I do anything else with an index range?  Can I access its 
members?  Can I iterate over it?


> Extension type is whatever is returned by the .extension() function (this
> is like .size() but returns a range)

And it apparently has a member function "elements" that is completely 
undocumented, given your earlier example:

 > for(auto indices : a.extensions().elements() ) {
 >       apply(a, indices) = get<0>(indices) + 10*get<1>(indices);
 > }


>> The return types of boost::multi::subarray::elements and
>> boost::multi::array_ref::elements should be documented.
>>
> 
> Well the type is a complicated type, that for the most part you don't need
> to know about.
> 
> multi::elements_range_t<int*, multi::layout_t<2, long int> >
> 
> It depends on other template parameters. It suffices to know that is a
> random access range.

So it meets the requirements of std::ranges::random_access_range.  Does 
it provide indexed access through operator[]?  Does copying it produce a 
deep or a shallow copy?  Is it returned by value or reference?

I don't care about the exact type.  I care about what properties the 
type has.

>> boost::multi::restriction has a member function called "home" which
>> "returns an indexable cursor".  I don't know what this cursor is, what
>> it does, or what members it has.
> 
> Cursors are described in other sections.
> I will add a section on the tutorial, its uses are pretty advances.
> For arrays, It is for context in which you can't use pass-by-reference.

I see a very brief mention of cursors in the Interoperability/CUDA 
section (which I skipped over because I am not familiar with CUDA). 
They are described as "a multidimensional generalization of iterators". 
Based on that description, I would expect being able to point cursors at 
different parts of the array (possibly by adding N-dimensional vectors 
to the cursor), but there is no such example given.  They are also 
described as references to arrays that can be indexed and cheaply 
copied, but subarray and array_ref can already be used for the same purpose.

Regardless of the purpose of cursors, I expect them to be documented in 
the reference section.  Other sections of the documentation are fine for 
what they are, but the reference section should be able to stand alone.

> It seems that the main technical criticism was regarding features that you
> can opt-in, like views, but that you are not forced to use.

I actually have fairly few technical criticisms.  My main problem is 
with the documentation is not only inadequate, but inadequate to the 
point where I can't even review key design elements without reading the 
source code because these design elements are not adequately documented. 
  For example, I feel that there should be bitwise operators in 
multi::elementwise for completeness, but I would have to read the source 
code to find out if they already exist or not.

The other main issue is naming things, which is of course one of the two 
hard problems in computer science.  Everything else is basically 
nitpicking on my part.

> Direct element access is something that you consider very useful and a
> deal-breaker for any multidimensional library.
> 
> I hope I convinced you that element address is a first class citizen in the
> library and it was a priority since day one.

Yes, the element address issue is mostly resolved.  I still don't like 
having to use "std::apply", but I can live with it.


-- 
Rainer Deyke - [email protected]

_______________________________________________
Boost mailing list -- [email protected]
To unsubscribe send an email to [email protected]
https://lists.boost.org/mailman3/lists/boost.lists.boost.org/
Archived at: https://lists.boost.org/archives/list/[email protected]/message/IZJ5TZKBPJAUJY24DPKZ6KGHD2SR2P5E/