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

Added is_finite check for 4x4 matrix inversion #1469

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

mat-kie
Copy link

@mat-kie mat-kie commented Jan 3, 2025

Fixes #1432

The issue is related to multiplications where the result exceeds the maximum value for the underlying numeric representation. See the added tests for example matrices that pass the determinant check but yield infinite values in the inversion code.

Changes:

  • Added a benchmark to compare LU decomposition performance against optimized 4x4 inversion code.
  • Added tests to ensure try_inverse correctly returns None for singular or near-singular matrices.
  • Added a convenience function for statically sized matrices in benchmarks (reproducible_matrix).

Notes:

  • The specialized 4x4 inversion routine was approximately 50 times faster than using LU decomposition.
  • Adding the is_finite check resulted in a 30% performance penalty (3ns instead of 2ns on my machine).
  • The check only avoids Inf or NaN values.
  • The issue from There's a potential bug with the Mat4 inverse code. #1432 probably also affects 2x2 and 3x3 matrices.
  • Consider adding the desired behavior for try_inverse to the method's documentation:
    • My proposal is to guarantee that the result only contains finite values if Some is returned. Other definitions would require more involved threshold calculations to define a bound on result precision.
  • I decided to leave the in-place inversion untouched and added context in the documentation.

@mat-kie mat-kie marked this pull request as draft January 3, 2025 23:06
@mat-kie mat-kie marked this pull request as ready for review January 3, 2025 23:24
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.

There's a potential bug with the Mat4 inverse code.
1 participant