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

Basic matrixcompare functionality #631

Merged
merged 4 commits into from Jul 17, 2020
Merged

Conversation

Andlon
Copy link
Sponsor Collaborator

@Andlon Andlon commented Aug 11, 2019

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.

Copy link
Member

@sebcrozet sebcrozet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

src/base/matrix.rs Show resolved Hide resolved
@Andlon
Copy link
Sponsor Collaborator Author

Andlon commented Aug 13, 2019

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.

@Andlon
Copy link
Sponsor Collaborator Author

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.

@sebcrozet
Copy link
Member

sebcrozet commented 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

@sebcrozet sebcrozet merged commit 2ab82be into dimforge:dev Jul 17, 2020
@sebcrozet
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants