-
-
Notifications
You must be signed in to change notification settings - Fork 485
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add macro for concatenating matrices #1080
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really cool! It's pretty close to what I was planning on doing in #446. I have some more features planned, but I think they can be added later without really breaking much (hopefully nothing).
I'm not entirely sold on special casing 1
to mean "identity". I think it's a bit too magical, especially since 1
is not the most common notation used to denote the identity matrix (although some authors/works do).
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![]
?
@sebcrozet: can you approve the CI, please?
src/base/dimension.rs
Outdated
type Output; | ||
|
||
fn unify(self, other: D) -> Option<Self::Output>; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we perhaps could just extend DimEq
with a representative() -> Option<Self::Representative>
method instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this is probably a better idea. I think it should be possible to do this with a default method to avoid a breaking change. With the same technique I think it should also be doable with a freestanding unify
function. I don't know what would be best though.
nalgebra-macros/src/lib.rs
Outdated
/// **Note: Requires the `macros` feature to be enabled (enabled by default)**. | ||
/// | ||
/// The syntax is similar to the [`matrix!`](./macro.matrix.html) and | ||
/// [`dmatrix`](./macro.dmatrix.html) macros. However the elements should |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's better to use an intra-doc link rather than directly linking to html
pages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Was this feature recently introduced? I don't think you could do this the last time I looked at how to link to other doc-pages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// ]; | ||
/// | ||
/// assert_eq!(a, b); | ||
/// ``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think most real-world use cases are going to be to concatenate matrices stored in variables rather than small matrices created with matrix!
, so it would be nice to have some examples showcasing this, e.g.
let matrix = cat![a, b;
c, 0];
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.
Would you be able to give some examples of this? Would be useful in the discussion.
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. |
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 |
This is because the result matrix is filled using
Agree. This seems like a good idea. I will look into good ways of doing this.
I was more thinking of the fact that by using
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 Mismatched dimensionscode: let m = cat![
&Matrix2::<usize>::identity(), 0;
&Matrix3::identity(), &Matrix2::identity();
]; error:
Failure to infer dimensionscode: let m = cat![
&Matrix2::<usize>::identity(), 0;
&nalgebra::SMatrix::identity(), &Matrix2::identity();
]; error:
|
Could you perhaps circumvent this by creating an intermediate 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
The simplest is probably to just somehow print the code generated by the macro, then take this code and paste it into a test.
Ah, that's a very good point. I personally think
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.
Interesting. The I don't have much experience with |
@sebcrozet: could you please approve the workflow? |
This was brought back to my mind through the |
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? |
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. |
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. |
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. |
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? |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the long delay @birktj. I'm still very busy but managed to squeeze in a little time to review this. I think the PR is already in a really good state, great work!
I have some relatively minor things left, please see comments.
I really think it would be great to make the macro work with non-reference types, and I added a concrete suggestion for how we might do that in the comments.
Other than that, I've been thinking that stack!
would be a better name that cat!
. What do you think?
Finally, even though you've done a good job with the current tests, I think given the importance of making sure this feature works correctly, I think we should have somewhat more exhaustive tests, including some "negative" tests that verify that the macro fails when given invalid input. See the "trybuild tests" already present for the matrix!
macro for examples of how this might be done.
It's also important to test simple corner cases, such as stack![]
or stack![a]
, stack![a, b]
and stack![a; b]
. Since this is a macro, we also need to verify that it works with variables or more complex expressions, such as stack![a + b, c; d - e, f]
or similar, or function calls etc.
I might also be able to contribute these myself, but I've unfortunately used up all my time budget in reviewing this (and other PRs), so I won't be able to do this for some time.
nalgebra-macros/src/lib.rs
Outdated
/// **Note: Requires the `macros` feature to be enabled (enabled by default)**. | ||
/// | ||
/// The syntax is similar to the [`matrix!`] and [`dmatrix!`]) macros. However the elements should | ||
/// be of type `&Matrix` or be one of the litterals `0` or `1`. The elements of type `&Matrix` are |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think, while using 0 for the zero matrix is OK, I think maybe using 1
for the identity matrix may be a little confusing. For example, if I see this:
let matrix = cat![1, a;
b, c];
I would be inclined to think that a
is a row vector and b
a vector, and 1
is a literal scalar entry. We could maybe instead introduce an identifier like Id
to serve as a placeholder for identity. So you would write
let matrix = cat![Id, a;
b, c];
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just removed the identity special case code. It might not really be very useful and thinking more about it I think any sort of special syntax will look a bit weird.
nalgebra-macros/src/lib.rs
Outdated
let start = (#row_offset, #col_offset); | ||
let shape = (#row_size, #col_size); | ||
let mut slice = matrix.generic_view_mut(start, shape); | ||
slice.copy_from(&nalgebra::Matrix::identity_generic(shape.0, shape.1)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rename slice
to view
|
||
#[allow(clippy::too_many_lines)] | ||
fn cat_impl(prefix: &str, matrix: Matrix) -> TokenStream2 { | ||
let n_macro_rows = matrix.nrows(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the purpose of prefix
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prefix
is so that we can have long complicated names for the internal variables that won't shadow external variables. This is really only relevant for the expr
and expr_shape
variables as they are introduced in an order where this could potentially happen.
nalgebra-macros/src/lib.rs
Outdated
ConcatElem::Zero => (), | ||
ConcatElem::One => { | ||
// FIXME: should be possible to use Matrix::fill_diagonal here, | ||
// but how to access `One::one()` hygienically? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can maybe just call nalgebra::one()
? Hopefully type inference should do the correct thing.
I have done most of what you asked for now:
|
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? |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing sticks out to me as an issue! I've provided a few comments but they're nitpicks. I think the state of this PR looks good so if you're also good with it @Andlon I can handle merging it.
/// If at least one element of a row has `Const<N>` number of rows then that row has a whole will | ||
/// have `Const<N>` number of rows. However if at least one row has has `Dynamic` number of rows | ||
/// then the entire matrix will have `Dynamic` number of rows. Same for columns. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit confusing; it took me a moment to parse it. I think I understand it after thinking it over, though. We might be able to clarify it a bit by differentiating between "rows" as rows of scalar in a matrix and "rows" as rows of matrices in the stack.
/// If at least one element of a row has `Const<N>` number of rows then that row has a whole will | |
/// have `Const<N>` number of rows. However if at least one row has has `Dynamic` number of rows | |
/// then the entire matrix will have `Dynamic` number of rows. Same for columns. | |
/// If at least one matrix in a row has `Const<...>` rows, then that row of the stack will have | |
/// a number of matrix rows known at compile time; if every row of the stack has a number | |
/// of matrix rows known at compile time then the resulting matrix will have a `Const<...>` | |
/// number of rows. However, if at least one row of the stack contains only matrices with | |
/// a dynamic number of rows, then the number of rows in the resulting matrix won't be | |
/// known at compile time, and so the entire matrix will be given a `Dynamic` number of | |
/// rows. The same applies to columns. |
t.compile_fail("tests/trybuild/stack_empty.rs"); | ||
t.compile_fail("tests/trybuild/stack_empty_row.rs"); | ||
t.compile_fail("tests/trybuild/stack_empty_col.rs"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be a good idea to add to this file tests to ensure that the macro doesn't explode if it's passed a matrix with zero rows or zero columns, e.g.
stack![
a, 0;
0, b;
];
Should yield a result equal to a
if b
is a dynamic matrix with 0 rows and 0 columns.
Actually, that's something that might be worth considering -- what should the behaviour of this macro be if passed a DMatrix
with zero in one dimension and nonzero in another dimension, and then asked to use 0
? E.g., the above, but the matrix b
has 2 rows and 0 columns. If a
were [1, 2; 3, 4]
then I think the most reasonable result would be
[
1, 2;
3, 4;
0, 0;
0, 0;
]
as that's the most predictable result of having that row and column count specified.
This also raises a question of whether a dynamic matrix with 3 rows and 0 columns should be considered "compatible" with a stack row that contains matrices with 5 rows. In theory it could be, although again raising an error might be the most predictable behaviour, as it lets us "take the dynamic matrix at its word" that it contains precisely 3 rows, even if it's got no entries.
@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). |
Thanks! This got merged as part of #1375. |
This PR introduces a
cat!
macro for concatenating multiple matrices.The
cat!
macro can be used like this:There are some points that probably should be addressed before this can be merged:
DimUnify
trait that is quite similar to theDimEq
trait with the main difference that it also can operate on values in addition to types with aunify
method. It might be possible to implement thecat!
macro without this trait.Matrix::uninitialized_generic
instead ofMatrix::zeros_generic
, however this would require generating unsafe code inside a macro which does not seem like a great idea.This fixes #446