Add macro for concatenating matrices

[birktj]

This PR introduces a cat! macro for concatenating multiple matrices.

The cat! macro can be used like this:

let a = cat![
    &matrix![1,2;3,4;], 1;
    0, &matrix![5,6;7,8];
];

let b = matrix![
    1,2,1,0;
    3,4,0,1;
    0,0,5,6;
    0,0,7,8;
];

assert_eq!(a, b);

There are some points that probably should be addressed before this can be merged:

• I have introduced a new DimUnify trait that is quite similar to the DimEq trait with the main difference that it also can operate on values in addition to types with a unify method. It might be possible to implement the cat! macro without this trait.
• Currently the macro generates quite a lot of code and is quite messy. Please come with feedback on the implementation of the macro.
• The code generated by the macro results in horrible error messages when it does not typecheck.
• The macro could be slightly more generic by using Matrix::uninitialized_generic instead of Matrix::zeros_generic, however this would require generating unsafe code inside a macro which does not seem like a great idea.

This fixes #446

[Andlon]

Currently the macro generates quite a lot of code and is quite messy. Please come with feedback on the implementation of the macro.

I think it would be useful to have some simple tests that actually test the macro output against expected code: this way it would be easier to get an overview over roughly how the generated code might look like. Currently this is hidden deep inside the implementation. Plus it would protect us a little from things that might be hard to catch with tests, such as potential issues with variable scoping etc.

The code generated by the macro results in horrible error messages when it does not typecheck.

Would you be able to give some examples of this? Would be useful in the discussion.

The macro could be slightly more generic by using Matrix::uninitialized_generic instead of Matrix::zeros_generic, however this would require generating unsafe code inside a macro which does not seem like a great idea.

I think this can be left as a future optimization, although personally I think we can save that for when someone actually complains about the performance.

[Andlon]

Also, before finally merging this PR later on I think we should have quite a bit more tests.

We should probably bikeshed over the name cat as well.. :upside_down_face:

[birktj]

From reading the code it wasn't immediately obvious to me, but why do matrices always have to be passed as references, i.e. &matrix![]?

This is because the result matrix is filled using Matrix::copy_from which takes the second matrix as a reference.

I think it would be useful to have some simple tests that actually test the macro output against expected code: this way it would be easier to get an overview over roughly how the generated code might look like. Currently this is hidden deep inside the implementation. Plus it would protect us a little from things that might be hard to catch with tests, such as potential issues with variable scoping etc.

Agree. This seems like a good idea. I will look into good ways of doing this.

The macro could be slightly more generic by using Matrix::uninitialized_generic instead of Matrix::zeros_generic, however this would require generating unsafe code inside a macro which does not seem like a great idea.

I think this can be left as a future optimization, although personally I think we can save that for when someone actually complains about the performance.

I was more thinking of the fact that by using Matrix::zeros_generic any matrix generated by cat! will have a T: Zero bound which may be unwanted in some situations.

Also, before finally merging this PR later on I think we should have quite a bit more tests.

I will have a look at this. Are there any particular tests that you think would be useful?

Some examples of bad error messages resulting from typing:

Seems like the errors are not as bad as I thought, I must have been thinking of the errors I got when debugging the macro. Sprinkling quote_spanned! here and there will probably also lead to some improvement.

Mismatched dimensions

code:

let m = cat![
    &Matrix2::<usize>::identity(), 0;
    &Matrix3::identity(), &Matrix2::identity();
];

error:

error[E0277]: the trait bound `Const<3_usize>: DimUnify<Const<2_usize>>` is not satisfied
   --> nalgebra-macros/tests/tests.rs:311:13
    |
311 |       let m = cat![
    |  _____________^
312 | |         &Matrix2::<usize>::identity(), 0;
313 | |         &Matrix3::identity(), &Matrix2::identity();
314 | |     ];
    | |_____^ the trait `DimUnify<Const<2_usize>>` is not implemented for `Const<3_usize>`
    |
    = help: the following implementations were found:
              <Const<A> as DimUnify<Const<A>>>
              <Const<A> as DimUnify<Dynamic>>
    = note: this error originates in the macro `cat` (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0277]: the trait bound `Const<2_usize>: DimUnify<Const<3_usize>>` is not satisfied
   --> nalgebra-macros/tests/tests.rs:311:13
    |
311 |       let m = cat![
    |  _____________^
312 | |         &Matrix2::<usize>::identity(), 0;
313 | |         &Matrix3::identity(), &Matrix2::identity();
314 | |     ];
    | |_____^ the trait `DimUnify<Const<3_usize>>` is not implemented for `Const<2_usize>`
    |
    = help: the following implementations were found:
              <Const<A> as DimUnify<Const<A>>>
              <Const<A> as DimUnify<Dynamic>>
    = note: this error originates in the macro `cat` (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0599]: no method named `generic_slice_mut` found for struct `Matrix<_, _, _, _>` in the current scope
   --> nalgebra-macros/tests/tests.rs:311:13
    |
311 |       let m = cat![
    |  _____________^
312 | |         &Matrix2::<usize>::identity(), 0;
313 | |         &Matrix3::identity(), &Matrix2::identity();
314 | |     ];
    | |_____^ method not found in `Matrix<_, _, _, _>`
    |
    = note: the method was found for
            - `Matrix<T, R, C, S>`
    = note: `cat![
                    &Matrix2::<usize>::identity(), 0;
                    &Matrix3::identity(), &Matrix2::identity();
                ]` is a function, perhaps you wish to call it
    = note: this error originates in the macro `cat` (in Nightly builds, run with -Z macro-backtrace for more info)

Some errors have detailed explanations: E0277, E0599.
For more information about an error, try `rustc --explain E0277`.
error: could not compile `nalgebra-macros` due to 3 previous errors

Failure to infer dimensions

code:

let m = cat![
    &Matrix2::<usize>::identity(), 0;
    &nalgebra::SMatrix::identity(), &Matrix2::identity();
];

error:

error[E0282]: type annotations needed
   --> nalgebra-macros/tests/tests.rs:311:13
    |
311 |       let m = cat![
    |  _____________^
312 | |         &Matrix2::<usize>::identity(), 0;
313 | |         &nalgebra::SMatrix::identity(), &Matrix2::identity();
314 | |     ];
    | |_____^ cannot infer type
    |
    = note: type must be known at this point
    = note: this error originates in the macro `cat` (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0599]: no method named `shape_generic` found for reference `&Matrix<_, Const<{_: usize}>, Const<{_: usize}>, ArrayStorage<_, {_: usize}, {_: usize}>>` in the current scope
   --> nalgebra-macros/tests/tests.rs:311:13
    |
311 |       let m = cat![
    |  _____________^
312 | |         &Matrix2::<usize>::identity(), 0;
313 | |         &nalgebra::SMatrix::identity(), &Matrix2::identity();
314 | |     ];
    | |_____^ method not found in `&Matrix<_, Const<{_: usize}>, Const<{_: usize}>, ArrayStorage<_, {_: usize}, {_: usize}>>`
    |
    = note: `cat![
                    &Matrix2::<usize>::identity(), 0;
                    &nalgebra::SMatrix::identity(), &Matrix2::identity();
                ]` is a function, perhaps you wish to call it
    = note: this error originates in the macro `cat` (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0599]: no method named `generic_slice_mut` found for struct `Matrix<_, _, _, _>` in the current scope
   --> nalgebra-macros/tests/tests.rs:311:13
    |
311 |       let m = cat![
    |  _____________^
312 | |         &Matrix2::<usize>::identity(), 0;
313 | |         &nalgebra::SMatrix::identity(), &Matrix2::identity();
314 | |     ];
    | |_____^ method not found in `Matrix<_, _, _, _>`
    |
    = note: the method was found for
            - `Matrix<T, R, C, S>`
    = note: `cat![
                    &Matrix2::<usize>::identity(), 0;
                    &nalgebra::SMatrix::identity(), &Matrix2::identity();
                ]` is a function, perhaps you wish to call it
    = note: this error originates in the macro `cat` (in Nightly builds, run with -Z macro-backtrace for more info)

Some errors have detailed explanations: E0282, E0599.
For more information about an error, try `rustc --explain E0282`.
error: could not compile `nalgebra-macros` due to 3 previous errors

[Andlon]

This is because the result matrix is filled using Matrix::copy_from which takes the second matrix as a reference.

Could you perhaps circumvent this by creating an intermediate MatrixSlice? Roughly:

let slice = matrix.generic_slice((0, 0), matrix.shape_generic());
// Slice is always a "matrix" so we can always take a reference to it and pass it in to copy_from
output_ij.copy_from(&slice);

This would however mean that this would compile (and work correctly):

let a = dmatrix![1];
let m = cat![ a ];
// can still use a, it wasn't *actually* moved
dbg!(a);

I don't think this is a problem, but it's obviously different from e.g. calling a function with a, in which case a would no longer be usable afterwards (unless Copy). Just wanted to point this out - I'm personally fine with this behavior. The behavior is otherwise the same as println!, e.g. println!("{}", a) does not drop a. I think that establishes enough precedent that we could go ahead with this.

Agree. This seems like a good idea. I will look into good ways of doing this.

The simplest is probably to just somehow print the code generated by the macro, then take this code and paste it into a test.

I was more thinking of the fact that by using Matrix::zeros_generic any matrix generated by cat! will have a T: Zero bound which may be unwanted in some situations.

Ah, that's a very good point. I personally think T: Zero is OK for now. We can lift it in the future if necessary.

I will have a look at this. Are there any particular tests that you think would be useful?

Mostly that they're somewhat "exhaustive", i.e. testing relatively exhaustively that we get the expected result for small matrix sizes. Especially corner cases like 0x0 or operations involving 0 or 1 dimensions. We also need some negative tests: if giving invalid input it's vital that the method correctly detects this and fails/panics.

Some examples of bad error messages resulting from typing:

Seems like the errors are not as bad as I thought, I must have been thinking of the errors I got when debugging the macro. Sprinkling quote_spanned! here and there will probably also lead to some improvement. Mismatched dimensions Failure to infer dimensions

Interesting. The mismatched dimensions example does indeed suggest that maybe DimEq would be nice, because it would complain that e.g. DimEq<Const<2>> is not implemented for Const<3>, which is perhaps clearer than DimUnify.

I don't have much experience with quote_spanned!, but if it's possible to use it to show which matrix is the "offending matrix", then that would be absolutely amazing :thinking: But it's not a blocker for merging, we might save this for future work as well. Having a working macro is an important first step, regardless if the error messages are less than optimal or not.

[Andlon]

@sebcrozet: could you please approve the workflow?

[birktj]

This was brought back to my mind through the hstack/vstack PR. I am still interested in completing this and getting this PR merged. However I believe my work stopped up partially because I am not completely sure what to do next. So does anyone here have any idea of what the blockers on merging this are?

[Andlon]

This was brought back to my mind through the hstack/vstack PR. I am still interested in completing this and getting this PR merged. However I believe my work stopped up partially because I am not completely sure what to do next. So does anyone here have any idea of what the blockers on merging this are?

Great to hear you're still interested in working on this! I think in terms of what's left, I think maybe the tests I suggested would be the most important. What's your opinion on the other comments that I made?

[Andlon]

Update: Sorry, I didn't see that you had in fact added some tests. I'll try to find time to look at them soon, but I'm very strapped for time at the moment, probably not before after the weekend.

[Andlon]

Thanks for working on this @birktj. Life and work are throwing a bit much at me right now, I hope I'll find time this week or next to review.

[Andlon]

Perhaps you could explicitly let me know when you feel that it is ready for review (if it isn't already)? So that I don't start reviewing before you feel that it's ready.

[birktj]

I think it should be ready for review now, but there is no hurry so take your time. It seems like the one failing build is unrelated to my changes?

[Andlon]

Hi @birktj, just want to let you know that I have not forgotten your PR here. Next to illness and being swamped at work, I haven't been able to devote any time to this yet. If you haven't already done so, it would help me a lot if you could review your own PR. That way maybe you can already detect some things that are worth pointing out or addressing, which might save me a little time once I manage to find the time to look at it.

[birktj]

I have done most of what you asked for now:

• I renamed cat to stack, I don't have strong opinions on the name and I think stack is perfectly fine.
• I removed the identity/1 special handling completely. I don't know if it is really that useful and it can be re-added at a later date if anyone wants.
• I fixed it so that both matrix references and values can be used, like you suggested.
• I added some more tests, including trybuild test that tests the three cases when the proc-macro should return with and error.

[birktj]

How are you doing @Andlon? I'm looking through some old issues and PRs and I see that this is still open. Do you have any time these days to have a look at my latest changes?

[Andlon]

Hi @birktj, contrary to what it might look like, I haven't forgotten about this PR. Due to paper and proposal deadlines, I am simply not able to allocate any time for open source projects other than what is strictly necessary for progress in my current research projects. I had hoped that this situation would have resolved itself much sooner, but I've been running into a series of unexpected problems with our current project, which has caused a significant delay compared to our original plan, which has left me in a bit of a tight spot. I hope this changes soon.

I really appreciate your work on this, and I'd love to get this merged soon - I just cannot say when I'll be able to do it myself. It will still be some time, unfortunately. In the meantime, it'd be great if someone else is also able to pitch in with a review, just to see if something sticks out perhaps.

[tpdickso]

@Andlon Any additional comments before this gets merged?

[Andlon]

@Andlon Any additional comments before this gets merged?

I actually decided that rather than pester @birktj for more work after all this time, I directly start continuing on his work myself, here: https://github.com/Andlon/nalgebra/tree/stack

Mainly much more extensive testing (still WIP), refactoring and I'm adding some more comments to explain what the code does (took me some time to parse since I'm not so used to working with macros). I hope to be done soon-ish, in which case I'll send an updated PR (of course including @birktj's original commits).