Improved stack! implementation, tests

[Andlon]

This PR improves upon #1080 with improved docs, extensive tests, and better UX through more accurate macro spans. The implementation of the stack! macro itself is hopefully also now a little bit easier to grok. The additional tests also uncovered an issue with the macro for accepting more complex expressions, which has now been fixed.

Big thanks to @birktj for the initial implementation, which was really clever. His commits are of course preserved in this PR.

I'm excited to hopefully soon land this feature, it's been a long while in the making. Once again my apologies in leaving you waiting for so long @birktj.

Once this PR is merged, we should also update the user guide to include guidance on constructing block matrices with stack!. I also want to create an additional issue that proposes to extend the stack! macro so that it can support diagonal concatenation and dynamic stacking by enabling slices of matrices to be valid inputs, in line with some of the ideas initially discussed in #446.

[Andlon]

I just remembered that I wanted to change the default prefix used for the macro. I'll also do a review of the PR myself before assigning a reviewer.

[Andlon]

Note that this PR introduces some minor breaking changes by providing new trait methods with default impls:

• DimEq::representative
• SameNumberOfRows::representative
• SameNumberOfColumns::representative

Since they have default impls, they're very unlikely to break anyone's code (but it's possible in rare circumstances). Technically, it would be possible to implement stack! without these changes, by instead providing a free-standing function, but these functions anyway seem to be a natural fit, as @birktj originally suggested in #1080.

I think in order to avoid breakage with the dimension traits, we should seal all of them. I have yet to see a legitimate use case for implementing any of the dim traits (DimEq, DimAdd etc.), so it would give us more room to maneuver if we sealed them all. But this is for another PR...

[Andlon]

Assigning you for review due to your prior involvement in #1080, @tpdickso. Please let me know if you're not able to.

Because I refactored some code in nalgebra-macros, and also moved a bunch of tests from nalgebra-macros tests to nalgebra tests, the diff in GitHub is unfortunately somewhat hard to read. Let me know if you want me to try to re-organize the PR. Reading the commit messages might also give you an idea of what was done, as they're supposed to tell a more or less coherent story.

[Andlon]

Just realized I hadn't tested any other built-ins than a single integer type. Added some basic sanity tests to ensure that stuff works as expected for the types we might care about (and by extension, it ought to work for any other arbitrary types).

[el-hult]

Nice! Looking forward to this addition!

One comment for posterity that might be useful when discussing terminology and/or adding documentation. I was confused about stacking vs concatenation as a new numpy user, coming from matlab. I can foresee the same confusing arising here as well.

Matlab use cat for concatenations, with the horzcat and vertcat for horizontal and vertical concatenations specifically. You can horzcat and vertcat vectors as well, since all Matlab arrays are at least 2d. Construction of block matrices (like stack! in this PR) is via repeated horzcat and vertcat.

Numpy (and other python nd-array manipulation libs) use np.concatenate to mean extending over an existing axis, and np.stack when creating a new axis. Numpy also provide np.hstack an np.vstack that operate on vectors or matrices, and does concatenation or stacking as needed. This is to be consistent with the Matlab API for horzcat and vertcat. The operation in this PR, called stack! is covered by np.block, as in constructing "block matrices".

Julia also allows nd-arrays, with a generic cat function that can conatenate along existing axes, and a stack function that create new axes. They provide hcat and vcat for specifically concatenating matrices and vectors horizontally and vertically, but they don't do stacking, breaking the Matlab semantics. The function hvcat corresponds to stack! of this PR by concatenating both horizontally and vertically.

Since nalgebra only have two-dimensional objects (vectors are matrices as well!) this means that the semantics are like in Matlab. But while the semantics are concatenations, the term chosen is stack!.

---

PS. I personally don't care about the chosen term, as long as I manage to find it and use it. stack! is as good as any. If anything, I kind of like tile for this operation... 🤷‍♂️

[Andlon]

Thanks for the useful comparison with other libs/languages @el-hult! Based on your summary, I think discoverability is key. I think updating the user guide with a chapter on block matrix construction / concatenation will hopefully go a long way towards that.

Here are the reasons I/we went with stack! for the naming:

• concatenate! is too long
• concat! is already an std macro
• cat! is just weird, it makes no sense on its own (unless you already know precisely what it does)
• stack! is:
  • short
  • a verb that communicates what happens to the matrices passed to it (unlike e.g. block!)
  • used extensively in various academic literature to describe this very process, in my experience

tile! is an interesting suggestion, but since it's not used anywhere else it's perhaps somewhat less intuitive than stack! (many people will already have run into vstack or hstack or similar).

[tpdickso]

This is fantastic! The code, docs, and tests are great. No issues, just a formatting nit.

[tpdickso]

rustfmt might have done something odd here, putting the first row of entries on a newline from the opening bracket might fix it!

[Andlon]

Good catch!

[Andlon]

Thanks for the review @tpdickso. I fixed the formatting issue you pointed out.

As I opened the code, the format_ident! calls stood out to me like a sore thumb. I changed them to read e.g. format_ident!("{prefix}_stack_{i}_{j}_block") instead of the previous format_ident!("{}_stack_{}_{}_block", prefix, i, j). This merely improves readability of the implementation of stack!, with no other changes. No further review required.

I think with this we're ready to merge. We don't yet have a clear routine for when to merge what (e.g. if the next release is a patch or a minor version bump etc.), so we'll have to figure out the plan for the next release(s) first.

[Ralith]

Cargo conventions leave it up to project discretion as to whether defaulted trait items are breaking. Given that people aren't going to be implementing the traits noted in #1375 (comment) on their own types, or calling those methods, very often themselves, I think it's reasonable to judge this non-breaking, if there's nothing else.

[Andlon]

Cargo conventions leave it up to project discretion as to whether defaulted trait items are breaking. Given that people aren't going to be implementing the traits noted in #1375 (comment) on their own types, or calling those methods, very often themselves, I think it's reasonable to judge this non-breaking, if there's nothing else.

Completely agree. I think a more useful label here would be minor-version to indicate that merging this PR requires a minor version bump.