/ nalgebra Public

Basic matrixcompare functionality#631

Merged
merged 4 commits into from
Jul 17, 2020
Merged

Basic matrixcompare functionality #631

merged 4 commits into from
Jul 17, 2020

Conversation

This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters

Note: this is not ready for merging. I'm posting it here for some initial feedback.
Update: This is in principle finalized and ready for merging (barring minor issues, see below).

This PR provides `matrixcompare` functionality for `nalgebra`. `matrixcompare` is a crate which aims to provide generic functionality for debug comparisons of matrices. A consumer crate like `nalgebra` only needs to define how many rows/columns a matrix has and how to fetch coefficients. Macros like `assert_matrix_eq!` then makes it easy to compare matrices with different kinds of comparators (absolute tolerance, relative tolerance etc.), which significantly simplifies debugging when writing unit tests involving linear algebra code.

`matrixcompare` is the result of extracting, rewriting and generalizing similar functionality found in the `rulinalg` crate (and also originally contributed by me).

Note that `matrixcompare` is currently not published, and is also not complete. Obviously, this will have to be remedied before this PR can be merged. I open it now so that I can perhaps have some initial feedback (including CI runs).

Below is an excerpt from one of the new tests.

```let a = MatrixMN::<_, U4, U5>::from_row_slice(&[
1210, 1320, 1430, 1540, 1650,
2310, 2420, 2530, 2640, 2750,
3410, 3520, 3630, 3740, 3850,
4510, 4620, -4730, 4840, 4950,
]);

let b = MatrixMN::<_, U4, U5>::from_row_slice(&[
1210, 1320, 1430, 1540, 1650,
2310, 2420, 2530, 2640, 2750,
3410, 3520, 3630, 3740, 3850,
4510, 4620, 4730, 4840, 4950,
]);

assert_matrix_eq!(a, b);```

And this is the output (format subject to change) at the time of writing:

``````Matrices X and Y have 1 mismatched element pairs.
The mismatched elements are listed below, in the format
(row, col): x = X[[row, col]], y = Y[[row, col]].

(3, 2): x = -4730, y = 4730.

Comparison criterion: exact equality x == y.
Please see the documentation for ways to compare matrices approximately.
``````

requested changes

Thanks, that looks very useful!

Why is the message method called `panic_message` at some places? I guess this does not panic, so would it make more sense to call this something like failure_message?

Or perhaps `compare_matrices` could return a `Result<(), MatrixComparisonFailure>`, where `MatrixComparisonFailure` would have its `Match` variant removed, and would implement `Display` and `Error`?

Show resolved Hide resolved

 Thanks for having a look! You're right, `panic_message` is a terrible name. It's somewhat of an old relic, in which it was used exclusively to panic inside of a macro. But I somehow didn't change the name as I was redesigning the functionality. I like your suggestions, I will try to apply them next chance I get! EDIT: Also thanks for your suggestion about copy instead of clone. I somehow switched around the name in the commit messages though, oops. Will fix this later too.

``` Basic matrixcompare functionality ```
``` f6730da ```
``` Improve matrixcompare example ```
``` 9196759 ```
``` Add compare feature to tests run by Makefile ```
``` 217a2bf ```

Andlon commented Jun 29, 2020

 @sebcrozet: I finally managed to revisit this PR. I made the changes you suggested earlier, and matrixcompare has now been released on `crates.io`. I've updated this PR to use the updated version. `matrixcompare` also supports sparse matrices (and dense-sparse comparisons of course), which is why I'm trying to get this across the finishing line now. It makes it much easier to write tests for my upcoming sparse contribution. This PR only includes support for `nalgebra` dense matrices, however. I will include sparse support in the upcoming CSR/CSC redesign. The `matrixcompare` crate is organized in terms of the core traits, that reside in `matrixcompare-core`, and further re-exported in the `matrixcompare` crate. The idea here is that libraries like `nalgebra` need only rely on `matrixcompare-core` (which has no dependencies and is expected to need updates very rarely, reducing dependency breakage), whereas consumers (e.g. nalgebra's own test suite) rely on whatever version of `matrixcompare` that they want to in their tests. I'm not sure how to really do it with features etc. I added a "compare" feature which would toggle the `matrixcompare-core` dependency. I cannot call it "matrixcompare", because `matrixcompare` is a dev-dependency already, which causes a conflict. Also, unfortunately I cannot toggle this feature for our tests by default, but I'm sure you're aware of this particular problem. I see that the CI fails for `no-std` tests. Not exactly sure how things fit together here, I have basically almost zero experience with `no-std`.

mentioned this pull request Jul 16, 2020

 Thank you for this PR! The CI failure when targeting `no-std` is caused by the `matrixcompare` dev-dependency which itself depends on `num-traits` which has its `std` dependency enabled. Unfortunately, cargo will unify enabled features for all dependencies, including dev-dependencies, when building the crate. So if `num-traits/std` is enabled on one dev-dependency, it will be enabled for all builds of `nalgebra` itself. This should be fixed by Andlon/matrixcompare#2

``` Use matrixcompare 0.1.3 for tests (fixes no-std test issues) ```
``` d13b3de ```
approved these changes
merged commit `2ab82be` into dimforge:dev Jul 17, 2020

sebcrozet commented Jul 17, 2020

 Thanks!