⚡ Optimize Matrix Determinant Calculation (O(N!) -> O(N^3)) - #7
Conversation
Co-authored-by: Inmerson <216765991+Inmerson@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Pull request overview
This PR replaces the determinant implementation in utils/matrixMath.ts with an O(N^3) Gaussian-elimination-based approach (partial pivoting) to avoid the exponential slowdown of Laplace expansion.
Changes:
- Replaced recursive Laplace expansion determinant calculation with Gaussian elimination + partial pivoting.
- Avoided mutating the input matrix by operating on a copied matrix.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (Math.abs(tempM[i][i]) < 1e-10) return 0; // Singular matrix | ||
|
|
There was a problem hiding this comment.
The singular-matrix check uses a fixed absolute threshold (1e-10). This is scale-dependent and can incorrectly return 0 for non-singular matrices with small pivots (or fail to detect singularity for very large-valued matrices). Consider using a relative tolerance based on the matrix scale (e.g., compare pivot to maxAbs in the column/row or to a norm times Number.EPSILON) and/or make the epsilon a named constant configurable by callers.
💡 What: Replaced the recursive O(N!) determinant calculation with an O(N^3) Gaussian elimination algorithm with partial pivoting in
utils/matrixMath.ts.🎯 Why: The recursive implementation became exponentially slower as matrix size increased, making it unusable for matrices larger than 10x10. 10! operations is ~3.6 million, while 10^3 is only 1000.
📊 Measured Improvement:
Measured on a 10x10 matrix:
Measured on a 8x8 matrix:
Correctness was verified against the original implementation for small matrices and standard edge cases (identity, singular).
PR created automatically by Jules for task 8802337678842268417 started by @Inmerson