Hi Everyone, Given the recent announcement of the upcoming Boost.Multi review, I wanted to offer some initial feedback. I would like to thank Alfredo for writing and sharing the library. I would also thank you for the single header version and the Compiler explorer integration. This is so welcoming not only for the users but also for the reviewers. I am not well familiar with the domain.i only had a look at the docs and played with toy examples in Compiler Explorer. I would like to request a Design Rationale section that would address some of the questions up front and bring some insight into the domain: 1. Is it a common need in the mentioned application fields to add a new dimension to the already stored N-dimensional data? If so, could we see an example of how to do it? 2. Why is c++17 chosen as the minimum? 3. The number of dimensions is static, but the sizes in each dimension are dynamic. Can you provide the rationale for this choice? The design: 1. I am still trying to digest the way by-reference and by-value is handled in the library in the presence of the "ref"/"view" types. It looks like the simple advice for the new users would be: * Always use `auto& ref = x` if you need the reference semantics, where `x` is whatever expression. * Always use `auto val = +x` if you need value semantics, where `x` is any expression. It looks like it is optimum even when `x` is of type `array`. 2. The initialization from values is too similar to the initialization from extents. I cannot easily figure out which initialization is used in `multi::array a({3, 4, 5});`. This is error-prone. I would prefer a longer: `multi::array a(multi::extents{3, 4, 5});`. On the other hand, does the initialization from small manual static sets of data happen often? I would think that for these use cases you would read the data from some external source. 3. If the docs recommend that all functions should be generic to account for subarrays and refs, then this library could offer a concept like `Indexable<Dimensionality, ElementType>` available for C++20 compilers. 4. Are sizes equal to strands? Does .elements() offer contiguous access? 5. For the initialization, did you consider factory functions or tag parameters? ( https://akrzemi1.wordpress.com/2016/06/29/competing-constructors/). The differentiation of meaning by braces/parens seems easy to misinterpret. 6. All the subarrays and dynamic_arrays have function swap, but it is not documented. Does it have a precondition that sizes should match? Documentation: 1.It is very clear already! 2. In the introduction it is worth explaining the term "stride-based layout" or link to the definition. This concept seems crucial to the library. 3. The docs say "or tensors in the 3D case". Tensors are arbitrary dimension. 4. I think that in real, practical use cases the array data is populated from files or datastres rather than from literals (compile-time values in braces). Could the tutorial demonstrate the endorsed way of doing such data population with this library? 5. I really appreciate that you prefix names with `multi::`. The examples could also mention the headers that you need to include. The initial docs examples may be the place where I will learn the headers. 6. The interface discoverability is not great: I wanted to quickly check the contract of `array::elements()`. I cannot find it. The reference section only has repeated text "same as for dynamic_array" (no link). Generally, the "redirect" philosophy in the reference documentation is mean.In order to look up the semantics of array::operator(), I am redirected: first from `array` to `dynamic_array`, then to `array_ref`, and only then to `subarray`. 7. Generally, we need to see a more detailed Reference section. For instance, what is the return type of operator[]? 8. Document if `array` and siblings can have dimensionality 0 (which would mean access to a single element). 9. It is strange to see a section titled "Static arrays" that describes type `dynamic_array`. In C++ "static" and "dynamic" tend to have opposite meanings. Regards, &rzej;
participants (1)
-
Andrzej Krzemienski