Implement simple macro for matrix creation

[Andlon]

There is no issue for this, but I've been missing a simple macro for creating small matrices. So I wrote a very simple one. It works like this:

let mat = matrix!(1.0, 2.0, 3.0;
                  4.0, 5.0, 6.0;
                  7.0, 8.0, 9.0);

As I'm sure you can infer from the example, you delimit columns by commas and rows by semi-colons, which is also the way MATLAB does it, except I chose to make the commas compulsory. Since I'm simply converting the expression to a Rust nested array, you get compiler errors if your matrix dimensions don't match, which is an advantage compared to constructor methods (which would also need you to specify rows and cols).

Such a macro is useful not just for ourselves when testing rulinalg, but for users of rulinalg who need to create small matrices when writing tests of their own applications.

What do you think?

[Andlon]

I haven't written any documentation. If you want the macro in the library, where do you want me to write the documentation for it?

[AtheMathmo]

Thanks for working on this! It actually already exists but isn't exposed.

The implementation is very similar to yours on the surface but the underlying macros are different. I wrote the existing one quite a while ago (my first ever macro) so yours may be significantly better!

I would like to see this available for users! I think the best approach to take would be to move your implementation and tests into the existing macros module. We should then make that module private but reexport in the matrix module.

[Andlon]

Oh, I didn't realize! Yeah, this was also the first time I touched Rust's macro system. Yours seems to be significantly more advanced than mine, although from your own comment in src/macros.rs they seem to have the same feature set at the moment. The macro I proposed is perhaps better in its simplicity, as most of it is simply straight-up regular Rust code. However, if you're still planning on the extensions you've mentioned in src/macros.rs, then there may be merit to choosing your implementation. If not, I suggest we swap it with mine.

[AtheMathmo]

I don't really remember the motivation behind much of the complication in mine - because of this we should probably go with yours.

Implementing the extensions I describe in that module are probably impossible? I'm not too fussed about those right now. I think the macros are most valuable for small matrices as you say.

For the documentation I think we could include an example in the crate documentation. And then just document the macro itself well - like vec!.

[Andlon]

@AtheMathmo: I made the requested changes. Was this what you had in mind...?

[AtheMathmo]

I believe the macro will not fail if the dimensions do not match?

In fact - it is possible to create a matrix that doesn't match the expected dimensions?

// Will think cols = 2, rows = 3 - 6 elements
matrix!(1.0, 1.0; 2.0, 2.0, 2.0; 3.0)

I'm not super fussed about this fail case - though I think it might be why I had the more complex setup before - to count the elements in each row and ensure they were equal (it looks incomplete?)

If I am misunderstanding and it does indeed fail - we should totally add a test for this as it is a behaviour I would love to keep!

[Andlon]

It does actually fail at compile time. I made a quick hack by pasting your example into the matrix_macro() test:

{
    // Will think cols = 2, rows = 3 - 6 elements
    let mat = matrix!(1.0, 1.0; 2.0, 2.0, 2.0; 3.0);
}

and I get the error:

src/macros.rs:51:45: 51:56 error: mismatched types [E0308]
src/macros.rs:51             let data_as_nested_array = [ $( [ $($x),* ] ),* ];
                                                             ^~~~~~~~~~~
src/macros.rs:107:23: 107:60 note: in this expansion of matrix! (defined in src/macros.rs)
src/macros.rs:51:45: 51:56 help: run `rustc --explain E0308` to see a detailed explanation
src/macros.rs:51:45: 51:56 note: expected type `[_; 2]`
src/macros.rs:51:45: 51:56 note:    found type `[_; 3]`
src/macros.rs:51:45: 51:56 error: mismatched types [E0308]
src/macros.rs:51             let data_as_nested_array = [ $( [ $($x),* ] ),* ];
                                                             ^~~~~~~~~~~
src/macros.rs:107:23: 107:60 note: in this expansion of matrix! (defined in src/macros.rs)
src/macros.rs:51:45: 51:56 help: run `rustc --explain E0308` to see a detailed explanation
src/macros.rs:51:45: 51:56 note: expected type `[_; 2]`
src/macros.rs:51:45: 51:56 note:    found type `[_; 1]`
error: aborting due to 2 previous errors
error: Could not compile `rulinalg`.

To learn more, run the command again with --verbose.

The macro basically converts the expression into a nested array (i.e. for 3x2 it will construct [ [ T; 2]; 3 ]) and relies on the fact that nested arrays can not have variable sizes. As far as I can tell, it is indeed impossible to construct a matrix whose dimensions are not compatible.

I didn't add a test for that, because I had no idea how to add a test for breaking the compilation. I can go ahead and add the compiletest-rs crate, if you want!

[Andlon]

Note that the error is not particularly readable, but I don't think there's anything to do about that. However, if you read it closely, it does actually say that the dimensions are wrong, in the sense of arrays.

[AtheMathmo]

I think just having this fail at compile time is a big advantage! Even if the error message is a little hard to parse (I find that most macro errors are very hard to decipher).

I haven't really looked into the compiletest-rs crate. If you're happy to do so I think it is worthwhile. I'll trust you if you say it is worth including (note that it should be a devDependency as in the crate README though).

Thanks!

[Andlon]

Sure, I can take a look (although not sure exactly when). Agree that it would be nice to be able to assert this behavior through tests!

[AtheMathmo]

The docs and everything look great. I have one comment which doesn't need any action. If it is an issue like I think, we can write this up and get it in as-is. If you are correct (I didn't actually test it...) - then I'd like to get a test to enforce this but this can wait too.

[Andlon]

@AtheMathmo: see my reply to the comment. I can add the compile-time test next chance I get to work on this. I think there's no particular hurry to get this in immediately, so we might as well get that test in proper first.

[AtheMathmo]

That's fine - whenever you have time.

I have one other question. Do you think it is worth emphasizing that this should be used for small matrices only? As we allocate the data on the stack first with the array and then copy it to the heap. Personally I think the documentation is fine right now and this should never really be a problem.

[Andlon]

I would hope that nobody in their right minds would populate a big matrix manually in their source code! That said, there may be the case where someone allocates small matrices in a tight loop... But then I think rulinalg is not the right library for them anyway, and they should use a library for low-dimensional linear algebra instead (which allocates full matrices on the stack for known matrix sizes).

But even then, considering the fact that the stack allocation is likely more or less free compared to the heap copy, and the fact that a regular construction of Matrix with its constructor would involve a copy to the heap (possibly from static storage, or maybe it even uses stack storage too, I have no idea), I'd be surprised if there is any noticeable performance difference at all! Of course, we'd have to bench that to check, but I think it's not something we have to worry about.

Just a small note: once this is in, perhaps we should try to make sure test data for new tests are written using the macro, because the compile-time checking might save us some future headache from accidental bugs such as mixing up Matrix::new(3, 2, ...) and Matrix::new(2, 3, ...) with the same data!

[AtheMathmo]

Ok, great! We'll keep the docs as is and refer to your comment in the future if this ever arises :) .

Just a small note: once this is in, perhaps we should try to make sure test data for new tests are written using the macro

I agree. I'll write up a ticket once this is merged to update existing tests where relevant too.

Edit: Just adding this to remind myself. This is waiting on research into compile-fail tests for matrix construction. We can merge happily without these if the effort/impact of adding them is too great.

[Andlon]

OK, so I spent about 30 minutes looking into compiletest. My impression is that it is not quite ready for primetime. I created the following "compile fail" test:

#[macro_use]
extern crate rulinalg;

fn main() {
    let mat = matrix!(0.0, 1.0; 2.0);
    //~^ error: mismatched types
    println!("{:?}", mat);
}

rustc gives me the following:

$ rustc -L ../../../../target/debug -L ../../../../target/debug/deps wrong_dimensions1.rs
error[E0308]: mismatched types
 --> wrong_dimensions1.rs:5:15
  |
5 |     let mat = matrix!(0.0, 1.0; 2.0);
  |               ^^^^^^^^^^^^^^^^^^^^^^ expected an array with a fixed size of 2 elements, found one with 1 elements
  |
  = note: expected type `[{float}; 2]`
  = note:    found type `[{float}; 1]`
  = note: this error originates in a macro from the standard library

error: aborting due to previous error

That looks good, right? Well, compiletest says the following:

$ cargo test
    Finished debug [unoptimized + debuginfo] target(s) in 0.0 secs
     Running target/debug/lib-95e6f24f32d274cc

running 4 tests
test mat::cholesky ... ok
test mat::qr ... ok
test mat::matrix_lup_decomp ... ok

running 1 test
test [compile-fail] compile-fail/matrix-macro/wrong_dimensions1.rs ... FAILED

failures:

---- [compile-fail] compile-fail/matrix-macro/wrong_dimensions1.rs stdout ----

error: tests/compiletests/compile-fail/matrix-macro/wrong_dimensions1.rs:5: expected error not found: mismatched types

error: 0 unexpected errors found, 1 expected errors not found
status: exit code: 101
command: rustc tests/compiletests/compile-fail/matrix-macro/wrong_dimensions1.rs -L /tmp --target=x86_64-unknown-linux-gnu --error-format json -L /tmp/matrix-macro/wrong_dimensions1.stage-id.compile-fail.libaux -C prefer-dynamic -o /tmp/matrix-macro/wrong_dimensions1.stage-id -L target/debug -L target/debug/deps
not found errors (from test file): [
    Error {
        line_num: 5,
        kind: Some(
            Error
        ),
        msg: "mismatched types"
    }
]

thread '[compile-fail] compile-fail/matrix-macro/wrong_dimensions1.rs' panicked at 'explicit panic', /home/andreas/.cargo/registry/src/github.com-1ecc6299db9ec823/compiletest_rs-0.2.2/src/runtest.rs:1084
note: Run with `RUST_BACKTRACE=1` for a backtrace.


failures:
    [compile-fail] compile-fail/matrix-macro/wrong_dimensions1.rs

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured

test compiletests::compile_test ... FAILED

failures:

---- compiletests::compile_test stdout ----
    thread 'compiletests::compile_test' panicked at 'Some tests failed', /home/andreas/.cargo/registry/src/github.com-1ecc6299db9ec823/compiletest_rs-0.2.2/src/compiletest.rs:120


failures:
    compiletests::compile_test

test result: FAILED. 3 passed; 1 failed; 0 ignored; 0 measured

error: test failed

Note the line where it says

error: 0 unexpected errors found, 1 expected errors not found

For whatever reason, I cannot get it to recognize any error at all. Also, note that I have to supply dependencies by -L target/debug -L target/debug/deps, which is very brittle in my opinion.

The idea is great, but it looks to me that the added brittleness and complexity isn't quite worth it for us yet. Maybe once it matures.

For completeness, here is the modified compiletest "runner" I used:

use std::path::PathBuf;

use compiletest::common::Mode;
use compiletest;

fn run_mode(mode: Mode, directory: &str) {
    let mut config = compiletest::default_config();
    config.mode = mode;
    config.src_base = PathBuf::from(format!("tests/compiletests/{}", directory));
    config.target_rustcflags = Some("-L target/debug -L target/debug/deps".to_string());

    compiletest::run_tests(&config);
}

#[test]
fn compile_test() {
    run_mode(Mode::CompileFail, "compile-fail");
}

[Andlon]

By the way, I used the nightly compiler for the above. I'd have to say, the error message from the matrix! macro is actually quite sensible! Pretty cool.

In any case, I suggest we merge this as it is for now. We should pay extra attention to any modifications done to matrix! for now, and then reinvestigate compiletest in the future. I'll make an issue on compiletest-rs for this issue, see what they say.

[AtheMathmo]

Thanks for taking the time to look into this. It's great that the nightly errors are even clearer!

I think you've made the right call. I'll merge this as it stands.

[Andlon]

Just leaving this here for future reference. I posted an issue to the compiletest-rs crate. Maybe depending on the outcome we can take this up again. I'm letting a compiletest branch remain on my fork of rulinalg from which we can continue work on this if deemed appropriate.