I think you got these already, but adding for posterity:
My notes:
-
Given that standard C++ already supports multi-dimensional arrays, I think it could be clearer in the Introduction what this library brings to the developer that is missing from the Standard. I would make the differences explicit - what features the Standard does not support. Perhaps this is just a wording change - from "Features of this library that aim to facilitate the manipulation of multidimensional arrays include:" to "Features of this library, that are not present in the Standard, that aim to facilitate the manipulation of multidimensional arrays include:". Though be careful to add further qualification to the features if any are present, in whole or in part, in the Standard. Is the Standard known for making "unnecessary copies" for example, if so, articulate this issue. Same deeper explanation for your other features.
-
In the Introduction again, I would have liked some more discussion on use cases. Multi-dimensional arrays are often used in real-time simulations for example, to provide physical data super-quickly without the need for equations or other computationally expensive approach. If there are other compelling examples (AI, finance, astrophysics, etc.), great to be specific and list them out. This does have the effect of drawing in developers working in those areas - particularly if you can link the unique features of the library mentioned above to those use cases.
-
Nit: typo - "althoug":
The library’s primary concern is with the storage and logic structure of data. It doesn’t make algebraic or geometric assumptions about the arrays and their elements. (althoug they are still good building blocks for implementing mathematical algorithms, such as representing algebraic dense matrices in the 2D case, or tensors in the general case.)
4. In the Intro there is this statement: Multi is a header-only library and C++17 or later is required.
Good info, but other requirements are scattered throughout the doc. Perhaps have a "Requirements" sub-heading and add OS/Compiler or other requirements? Later on in the doc there is: The code requires any modern C++ compiler (or CUDA compiler) with standard C++17 support; for reference,...
All statements like this belong in the Requirements section, which itself should follow the Introduction.
-
Nit. Odd, truncated sentence in the table section: Why is dimensionality static and the sizes are dynamic?
Sizes are fixed or
-
Navigation.
Scroll to the end of a page there is no navigation options - great to see "next" and "previous" page links. Currently a bit tedious to have to relocate my place in the TOC.
-
Almost no linking to outside sources. For example, all the following sentences could contain a link (as could many others):
Testing the library requires Boost.Core (headers),
The library works out-of-the-box in combination with the Thrust library.
The library works out of the box with Eric Niebler’s Range-v3 library,
CUDA is a dialect of C++ that allows writing pieces of code for GPU execution,
or with the standard MDSpan proposal std::mdspan.
In the next section instead we will see an example of arrays owned by the library. [link to next section where this example is]
-
Like the links to Compiler Explorer, but not the linking word "online" or "live" - doesn't say what the link is to - "Compiler Explorer" or whatever the title of the destination is would be much clearer.
-
Nits. Remove unnecessary (and slightly condescending) words - simple or a simple adds no meaning (same for the word "very"), two examples:
Once installed, other CMake projects (targets) can depend on Multi by adding a simple add_subdirectory ...
change this to:
Once installed, other CMake projects (targets) can depend on Multi by adding add_subdirectory ...
Same for the following, and any other use of simply or very:
—something manual indexing simply cannot do cleanly at scale.
-
Only add brackets where really necessary, they are an obstacle to smooth reading, such as here:
(Although most of the examples use numeric elements for conciseness, the library is designed to hold general types (e.g. non-numeric, non-trivial types, like std::string, other containers or, in general, user-defined value-types.)
A (mutable) array can be assigned
(The Cereal XML and Boost XML output would have a similar structure.)
(de)serializing [spell it out "when serializing or deserializing a ..."]- and many more examples.
-
Perhaps "Getting Started" is a friendlier title than Primer (basic usage) and "Tutorials" better than Tutorial (advanced usage). However, the tutorials are not really tutorials unless they contain numerical steps that a student can follow. As it is - Advanced Usage Examples, or a similar title, is more accurate.
-
Nit. Typo: Because this array is not controled by the library, this type of object is called an array-reference.
Correct spelling is controlled.
-
Nit. Typo: why the colon?
Note that this is also different from creating a named reference:
-
Style nit. Make reference material impersonal - not "In my experience, however, the following" more "Usage patterns can produce .....". Library documentation is formal and not a blog post, so "opinions" are mostly inappropriate - expressing factually with conditions is more appropriate.
-
Typo - sentence starting with lower case:
which uses the default Thrust device backend (
-
Typo - upper case C needed: Why is c++17 chosen as the minimum?
-
Not crazy about punctuation in headings, for example:
{fmt} - would be better as "Formatting"
Copy, and assigment (, and aliasing) is ugly, "Copy, Assignment and Aliasing" works better
Perhaps use Title case for headings - capitalize nouns, verbs, adjectives, not articles, prepositions or conjunctions. Avoid punctuation if at all possible.
18. Not sure about the following example code:
Compile time evaluation
constexpr auto trace() {
multi::array<int, 2> arr = {{1, 2, 3}, {4, 5, 6}, {7, 8, 9}};
arr[2][2] = 10;
return std::accumulate(arr.diagonal().begin(), arr.diagonal().end());
}
static_assert( trace() == 4 + 2 + 10 );
If arr[2][2] is 10, shouldn't the assert be "1 + 5 + 10", or have I misunderstood "diagonal"?
-
Reference - I agree with earlier comments that the Reference should be more complete. Each function/method should have its own page (or section) to include a brief sentence on its purpose, then syntax, parameters, return values, errors and exceptions, remarks for a fuller description if necessary, and for bonus points an example, or link to an example, of its use.
-
Appendix - How to's is not appendix material - belongs after or part of tutorials (perhaps renamed to "Advanced Use Cases" or similar).
I think you got these already, but adding for posterity:
My notes:
Given that standard C++ already supports multi-dimensional arrays, I think it could be clearer in the Introduction what this library brings to the developer that is missing from the Standard. I would make the differences explicit - what features the Standard does not support. Perhaps this is just a wording change - from "Features of this library that aim to facilitate the manipulation of multidimensional arrays include:" to "Features of this library, that are not present in the Standard, that aim to facilitate the manipulation of multidimensional arrays include:". Though be careful to add further qualification to the features if any are present, in whole or in part, in the Standard. Is the Standard known for making "unnecessary copies" for example, if so, articulate this issue. Same deeper explanation for your other features.
In the Introduction again, I would have liked some more discussion on use cases. Multi-dimensional arrays are often used in real-time simulations for example, to provide physical data super-quickly without the need for equations or other computationally expensive approach. If there are other compelling examples (AI, finance, astrophysics, etc.), great to be specific and list them out. This does have the effect of drawing in developers working in those areas - particularly if you can link the unique features of the library mentioned above to those use cases.
Nit: typo - "althoug":
The library’s primary concern is with the storage and logic structure of data. It doesn’t make algebraic or geometric assumptions about the arrays and their elements. (althoug they are still good building blocks for implementing mathematical algorithms, such as representing algebraic dense matrices in the 2D case, or tensors in the general case.)
4. In the Intro there is this statement: Multi is a header-only library and C++17 or later is required.
Good info, but other requirements are scattered throughout the doc. Perhaps have a "Requirements" sub-heading and add OS/Compiler or other requirements? Later on in the doc there is: The code requires any modern C++ compiler (or CUDA compiler) with standard C++17 support; for reference,...
All statements like this belong in the Requirements section, which itself should follow the Introduction.
Nit. Odd, truncated sentence in the table section: Why is dimensionality static and the sizes are dynamic?
Sizes are fixed or
Navigation.
Scroll to the end of a page there is no navigation options - great to see "next" and "previous" page links. Currently a bit tedious to have to relocate my place in the TOC.
Almost no linking to outside sources. For example, all the following sentences could contain a link (as could many others):
Testing the library requires Boost.Core (headers),
The library works out-of-the-box in combination with the Thrust library.
The library works out of the box with Eric Niebler’s Range-v3 library,
CUDA is a dialect of C++ that allows writing pieces of code for GPU execution,
or with the standard MDSpan proposal std::mdspan.
In the next section instead we will see an example of arrays owned by the library. [link to next section where this example is]
Like the links to Compiler Explorer, but not the linking word "online" or "live" - doesn't say what the link is to - "Compiler Explorer" or whatever the title of the destination is would be much clearer.
Nits. Remove unnecessary (and slightly condescending) words - simple or a simple adds no meaning (same for the word "very"), two examples:
Once installed, other CMake projects (targets) can depend on Multi by adding a simple add_subdirectory ...
change this to:
Once installed, other CMake projects (targets) can depend on Multi by adding add_subdirectory ...
Same for the following, and any other use of simply or very:
—something manual indexing simply cannot do cleanly at scale.
Only add brackets where really necessary, they are an obstacle to smooth reading, such as here:
(Although most of the examples use numeric elements for conciseness, the library is designed to hold general types (e.g. non-numeric, non-trivial types, like std::string, other containers or, in general, user-defined value-types.)
A (mutable) array can be assigned
(The Cereal XML and Boost XML output would have a similar structure.)
(de)serializing [spell it out "when serializing or deserializing a ..."]- and many more examples.
Perhaps "Getting Started" is a friendlier title than Primer (basic usage) and "Tutorials" better than Tutorial (advanced usage). However, the tutorials are not really tutorials unless they contain numerical steps that a student can follow. As it is - Advanced Usage Examples, or a similar title, is more accurate.
Nit. Typo: Because this array is not controled by the library, this type of object is called an array-reference.
Correct spelling is controlled.
Nit. Typo: why the colon?
Note that this is also different from creating a named reference:
Style nit. Make reference material impersonal - not "In my experience, however, the following" more "Usage patterns can produce .....". Library documentation is formal and not a blog post, so "opinions" are mostly inappropriate - expressing factually with conditions is more appropriate.
Typo - sentence starting with lower case:
which uses the default Thrust device backend (
Typo - upper case C needed: Why is c++17 chosen as the minimum?
Not crazy about punctuation in headings, for example:
{fmt} - would be better as "Formatting"
Copy, and assigment (, and aliasing) is ugly, "Copy, Assignment and Aliasing" works better
Perhaps use Title case for headings - capitalize nouns, verbs, adjectives, not articles, prepositions or conjunctions. Avoid punctuation if at all possible.
18. Not sure about the following example code:
Compile time evaluation
constexpr auto trace() {
multi::array<int, 2> arr = {{1, 2, 3}, {4, 5, 6}, {7, 8, 9}};
arr[2][2] = 10;
return std::accumulate(arr.diagonal().begin(), arr.diagonal().end());
}
static_assert( trace() == 4 + 2 + 10 );
If arr[2][2] is 10, shouldn't the assert be "1 + 5 + 10", or have I misunderstood "diagonal"?
Reference - I agree with earlier comments that the Reference should be more complete. Each function/method should have its own page (or section) to include a brief sentence on its purpose, then syntax, parameters, return values, errors and exceptions, remarks for a fuller description if necessary, and for bonus points an example, or link to an example, of its use.
Appendix - How to's is not appendix material - belongs after or part of tutorials (perhaps renamed to "Advanced Use Cases" or similar).