Skip to content
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

Matrix macro #886

Merged
merged 25 commits into from May 9, 2021
Merged

Matrix macro #886

merged 25 commits into from May 9, 2021

Conversation

@Andlon
Copy link
Collaborator

@Andlon Andlon commented May 3, 2021

This PR introduces four new macros to nalgebra. They are available under the (now default) macros feature, which makes nalgebra export the macros from the procedural macro crate nalgebra-macros. The macros are:

  • matrix![] for constructing fixed-size matrices with a MATLAB-like syntax.
  • dmatrix![] for constructing dynamic matrices with the same syntax as matrix!
  • vector![] for constructing small fixed-size vectors (syntax basically the same as vec!)
  • dvector![] for constructing dynamically allocated vectors with the same syntax as vector.

The intention here is to simplify matrix/vector construction by improving readability, ease-of-use and ensuring that the matrix construction is as fast as possible.

Example:

use nalgebra::matrix;

// Directly constructs an SMatrix<_, 2, 3>
let a = matrix![1, 2, 3;
                4, 5, 6];

Benefits over e.g. MatrixRxC::new constructors:

  • Number of rows and columns are inferred from the data, so that when changing the data (e.g. increasing column count) you don't need to update both the data and the type.
  • More succinct and obvious.
  • Can be combined with e.g. #[rustfmt::skip::macros(matrix)] to prevent rustfmt from messing up carefully aligned syntax.
  • Works for matrices of any (practical) size, not just those that have new constructors.
  • Semi-colons clearly delineate where each row begins.

Disadvantages:

  • Using the macros adds syn and quote to the dependency tree. On my 6-7 year old laptop, this adds ~10 secs to clean build compile time. Note that if you use serde or possibly some of the other optional dependencies, you might already have syn/quote in your dependency tree, so you pay effectively no penalty. If users don't need the macro, it's also possible to opt out by disabling the macros feature.
  • I can't think of any others, but please let me know if I've missed any.

The user guide should probably be updated to accommodate these macros. My suggestion is to make these macros the de-facto way of constructing matrices with nalgebra.

Resolves #873.

@Andlon
Copy link
Collaborator Author

@Andlon Andlon commented May 4, 2021

I'm missing one or more tests that verify that the macros work for all built-in types. I will add this as soon as I have time.

@Andlon
Copy link
Collaborator Author

@Andlon Andlon commented May 4, 2021

I'm also missing tests where instead of just numbers, we have arbitrary Rust expressions (though one of the doc tests has something like this). I will add this later too.

@Andlon Andlon marked this pull request as draft May 4, 2021
@Ralith
Copy link
Contributor

@Ralith Ralith commented May 4, 2021

What's the case for vector![1, 2, 3] existing as a distinct thing from matrix![1; 2; 3]?

@Andlon
Copy link
Collaborator Author

@Andlon Andlon commented May 5, 2021

What's the case for vector![1, 2, 3] existing as a distinct thing from matrix![1; 2; 3]?

Semantics, mostly. Although a vector is also a matrix, matrices and vectors often play different roles in applications, so at least in my opinion it benefits readability to distinguish them at the syntax level (although you can indeed use matrix! to construct vectors as well, if you are so inclined).

Second, dvector![1, 2, 3] and dmatrix![1; 2; 3] do not produce the same type. In the first case, you get DVector<T>, in the second case DMatrix<T>. It seems to me that for symmetry purposes it is then natural to have vector! mirror dvector! and matrix! mirror dmatrix!.

@Andlon Andlon marked this pull request as ready for review May 5, 2021
@Andlon
Copy link
Collaborator Author

@Andlon Andlon commented May 5, 2021

All right, I finished up the things I wanted to address, so I suppose this PR should be ready now.

@sebcrozet
Copy link
Member

@sebcrozet sebcrozet commented May 5, 2021

Hey @Andlon, this is an amazing PR. Thank you! I will review this within the next few days.

The user guide should probably be updated to accommodate these macros. My suggestion is to make these macros the de-facto way of constructing matrices with nalgebra.

I agree. I will take care of this later this month.

@Andlon
Copy link
Collaborator Author

@Andlon Andlon commented May 5, 2021

Thanks, @sebcrozet! I also wanted to add that I don't have much experience with proc macros, so if there's any experienced proc macro person with some good suggestions for improvement I'd also be happy to hear about that.

Copy link
Member

@sebcrozet sebcrozet left a comment

Thank you again for this PR. This looks great. I'm not a proc-macro expert myself so I can't really judge the quality of the implementation. But it looks OK to me.

src/base/array_storage.rs Outdated Show resolved Hide resolved
src/base/vec_storage.rs Outdated Show resolved Hide resolved
@Ralith
Copy link
Contributor

@Ralith Ralith commented May 6, 2021

The proc macro stuff LGTM.

@Andlon
Copy link
Collaborator Author

@Andlon Andlon commented May 7, 2021

@sebcrozet: I moved the impl blocks, but I needed to add some conditional compilation for the VecStorage-related stuff in matrix.rs. Not sure if this is the way you prefer it. Let me know if you want it done differently.

@Ralith: thanks for taking a look!

@sebcrozet
Copy link
Member

@sebcrozet sebcrozet commented May 9, 2021

@Andlon Looks greatk thanks!

@sebcrozet sebcrozet merged commit 23ac85e into dimforge:dev May 9, 2021
9 checks passed
9 checks passed
@github-actions
check-fmt
Details
@github-actions
clippy
Details
@github-actions
build-nalgebra
Details
@github-actions
test-nalgebra
Details
@github-actions
test-nalgebra-glm
Details
@github-actions
test-nalgebra-sparse
Details
@github-actions
test-nalgebra-macros
Details
@github-actions
build-wasm
Details
@github-actions
build-no-std
Details
@Andlon Andlon deleted the Andlon:matrix-macro branch May 10, 2021
@sebcrozet sebcrozet mentioned this pull request Jun 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants