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

Inverting a 4x4 matrix with try_inverse_mut doesn't leave self unchanged if the inversion fails. #1384

Merged
merged 8 commits into from
May 5, 2024

Conversation

jnyfah
Copy link
Contributor

@jnyfah jnyfah commented Apr 19, 2024

Fixes #1380

Modified the do_inverse4 function to calculate the determinant of 4x4 matrix before proceeding with the computation of the adjugate matrix to ensure that out is not modified if the determinant is zero.

@jnyfah jnyfah changed the title inversion Inverting a 4x4 matrix with try_inverse_mut doesn't leave self unchanged if the inversion fails. Apr 19, 2024
src/linalg/inverse.rs Outdated Show resolved Hide resolved
src/linalg/inverse.rs Outdated Show resolved Hide resolved
@jnyfah jnyfah requested a review from Ralith April 22, 2024 12:48
src/linalg/inverse.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@Ralith Ralith left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@jnyfah jnyfah requested a review from Ralith April 28, 2024 16:12
@Ralith
Copy link
Collaborator

Ralith commented Apr 28, 2024

I guess we don't have any unit tests validating this. Want to add one?

@jnyfah
Copy link
Contributor Author

jnyfah commented Apr 29, 2024

@Ralith, I realized that I had mistakenly omitted the cofactor out[(1, 0)] previously , which was causing the self_mul_inv_is_id_dim4 test to fail in the workflow. I've added it now.

@Ralith
Copy link
Collaborator

Ralith commented Apr 29, 2024

Oh, nice, looks good then.

@Andlon
Copy link
Collaborator

Andlon commented Apr 30, 2024

Thanks a lot @jnyfah! I'm just quickly reading over here, but it seems to me that we still don't have a test that would fail before this change, but now passes? I think this would be useful, and I think it's what @Ralith alluded to before. Perhaps I'm missing something though!

@jnyfah
Copy link
Contributor Author

jnyfah commented Apr 30, 2024

@Andlon oh yes, added the test now

#[test]
fn test_inversion_failure_leaves_matrix_unchanged(m in matrix4()) {
let original_matrix = m.clone();
if m.try_inverse().is_none() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this branch actually get hit? I'm not sure how often proptest generates non-invertible matrices. Would a hardcoded case work better?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agree with @Ralith, I think the likelihood of matrix4() producing singular matrices is likely (very) small (though I haven't checked), so a test with a fixed input is better in this case.

@Ralith
Copy link
Collaborator

Ralith commented May 1, 2024

I was actually just asking about a test to make sure this isn't causing a regression, but that was already covered.

@jnyfah
Copy link
Contributor Author

jnyfah commented May 2, 2024

@Ralith @Andlon added a test with a fixed noninvertible input

Copy link
Collaborator

@Andlon Andlon left a comment

Choose a reason for hiding this comment

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

Thanks for adding the test. We're almost there, I'm unfortunately going to have to be a little bit picky still :-) See comments!

4.0, 8.0, 12.0, 16.0
);
let expected = mat.clone();
if !mat.try_inverse_mut() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This test is currently very brittle. Imagine that some change in an implementation detail somewhere caused mat.determinant() to return something not exactly 0.0. Then mat.try_inverse_mut() might return true and the test would succeed, although it's no longer testing what we want to test it.

Perhaps replace the if by an assert? E.g. assert!(!mat.try_inverse_mut()) followed by the assert_eq that you already have.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Andlon fixed it

@@ -1263,6 +1263,20 @@ fn column_iterator_double_ended_mut() {
assert_eq!(col_iter_mut.next(), None);
}

#[test]
fn test_inversion_failure_leaves_matrix_unchanged() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe rename to _matrix4_unchanged to indicate that this was specifically an issue with 4x4 inversion and what we're testing here

@jnyfah jnyfah requested a review from Andlon May 3, 2024 18:58
@Andlon
Copy link
Collaborator

Andlon commented May 4, 2024

@jnyfah: thanks for fixing the last remaining points! We're all good to go now, except CI is failing due to formatting. I think you just need to run cargo fmt and it should be fine.

@Andlon Andlon merged commit afc03cc into dimforge:dev May 5, 2024
10 of 11 checks passed
@Andlon
Copy link
Collaborator

Andlon commented May 5, 2024

Thanks for your help with this @jnyfah! The CI failed due to an unrelated issue (a brittle test).

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.

Inverting a 4x4 matrix with try_inverse_mut doesn't leave self unchanged if the inversion fails.
3 participants