la.jacobi: fix incorrect eigen values/vectors #224
Conversation
WalkthroughThe Jacobi method implementation has been updated to adjust its control flow and numerical updates. The convergence check now uses a break statement instead of an immediate return, and the computation of the difference variable has been reversed. Additional tolerance conditions have been added before processing further calculations. The matrix update logic has been refactored using a single loop, and the final matrix assembly now explicitly sets both diagonal and off-diagonal elements. In parallel, a new test case for a 3x3 symmetric matrix and additional assertions in an existing test have been introduced. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller/Test
participant Jacobi as jacobi()
participant Matrix as Matrix (a, v, q)
Caller->>Jacobi: Call jacobi(q, v, a)
Note over Jacobi: Start iterations for Jacobi rotation
Jacobi->>Jacobi: Compute h = v[j] - v[i]
alt Tolerance Condition Met & Convergence Check
Jacobi->>Jacobi: Break loop
else Continue Calculations
Jacobi->>Jacobi: Calculate t based on h and a.get(i, j)
Jacobi->>Matrix: Update matrix values (aii, ajj, aij) and set a[i][j] to 0.0
end
Jacobi->>Matrix: Update remaining matrix entries (excluding i and j)
Jacobi->>Matrix: Set diagonal elements to v and off-diagonals to zero
Jacobi-->>Caller: Return updated matrices or error if not converged
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
la/jacobi_test.v (1)
35-63: Comprehensive test for a 3×3 symmetric matrix.Introducing this test function is an excellent step, as it covers a non-trivial use case. Consider expanding validations further (e.g., checking orthonormality of the eigenvectors) for even greater confidence in correctness.
la/jacobi.v (1)
131-139: Convergence verification after final assembly.Performing a final check on off-diagonal elements is good practice. For larger matrices or stricter needs, consider making
max_iterationsandtolexternally configurable to handle edge cases without modifying the source.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
la/jacobi.v(2 hunks)la/jacobi_test.v(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: run-tests-on-linux (ubuntu-22.04)
- GitHub Check: run-tests-on-linux (ubuntu-20.04)
- GitHub Check: run-tests-on-linux (ubuntu-24.04, --prod)
- GitHub Check: run-tests-on-linux (ubuntu-22.04, --prod)
- GitHub Check: run-tests-on-linux (ubuntu-20.04, --prod)
🔇 Additional comments (9)
la/jacobi_test.v (2)
22-26: Well-defined expected output for the diagonal matrix test.These lines correctly define the expected diagonal matrix, matching the behavior where no rotation is needed. Good job including these checks to ensure the final matrix remains unchanged.
32-33: Great addition of the final matrix assertion.Verifying that the resulting matrix
aremains unchanged intest_jacobi01ensures the algorithm’s correctness for trivial diagonal cases. This is an important validation step.la/jacobi.v (7)
65-65: Switch to break for convergence check is appropriate.Using
breakhere allows the method to proceed with final matrix setup and validations, which is beneficial compared to an immediate return.
71-74: Early continuation for negligible off-diagonal elements.Skipping the rotation when
math.abs(a.get(i, j))is below the tolerance is a valid optimization to avoid unnecessary computations while preserving accuracy.
77-77: Condition handles small differences robustly.Checking
math.abs(h) < tolin conjunction withmath.abs(a.get(i, j))accommodates tiny floating-point values, preventing numerical instability during rotation calculations.
90-99: Correct handling of updated diagonal and zeroed off-diagonal elements.Properly adjusting
a(i, i),a(j, j)and settinga(i, j)anda(j, i)to zero after computingtfollows the Jacobi rotation approach, preserving symmetry and accurately tracking eigenvalues inv.
101-117: Single-pass updates for matrix rows and columns.Applying the rotation in this unified loop ensures all relevant elements of
aandqare updated consistently. This is a clean and efficient implementation of the Jacobi rotation procedure.
120-120: No changes needed here.This is just the close of the nested loops. No issues are observed.
122-130: Final diagonalization step is standard Jacobi procedure.Setting
a(i, i) = v[i]while zeroing off-diagonal elements ensures that the final matrixais diagonal, matching the extracted eigenvalues. This step aligns with the classical Jacobi method conclusion.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
la/jacobi_test.v(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: run-tests-on-linux (ubuntu-20.04)
- GitHub Check: run-tests-on-linux (ubuntu-22.04, --prod)
- GitHub Check: run-tests-on-linux (ubuntu-22.04)
- GitHub Check: run-tests-on-linux (ubuntu-20.04, --prod)
- GitHub Check: run-tests-on-linux (ubuntu-24.04, --prod)
- GitHub Check: run-tests-on-macos (macos-latest, --prod)
- GitHub Check: run-tests-on-linux (ubuntu-24.04)
- GitHub Check: run-tests-on-macos (macos-latest)
🔇 Additional comments (2)
la/jacobi_test.v (2)
22-32: Good addition of matrix verification in the existing test.The addition of
expected_aand its verification ensures that the Jacobi algorithm not only produces correct eigenvalues and eigenvectors but also correctly updates the input matrixa. This directly addresses the issue mentioned in the PR where the original implementation failed to properly zero off-diagonal elements.
35-63: Well-structured new test case for a complex symmetric matrix.This is an excellent addition that tests the Jacobi algorithm with a non-trivial symmetric matrix. The test includes well-defined expected outputs for eigenvalues, eigenvectors, and the final state of matrix
a. This test case directly addresses the issues mentioned in the PR objectives, where the original implementation produced incorrect results for non-diagonal matrices.A few observations:
- The expected values have high precision, which suggests they were carefully calculated or verified against a reliable reference.
- The tolerance of 1e-14 for floating-point comparisons is appropriate for this type of numerical calculation.
- The test verifies all aspects of the algorithm's output: eigenvalues, eigenvectors, and the updated input matrix.
This PR resolves issue #202, where the Jacobi eigenvalue algorithm in
vsl.laproduced incorrect results. The original rotation logic failed to zero off-diagonal elements and broke the symmetry of the matrixa, resulting in incorrect eigenvalues (v) and eigenvectors (q).I retained the initial test with a diagonal matrix:
Expected :
v = [2, 2, 2],q = identity,aunchanged. It still passes.I added a new test for a more complex symmetric matrix, which was failing prior to the fix:
Expected :
v = [4.879385241571816, 2.652703644666139, 1.467911113762045],qmatches computed eigenvectors,adiagonalized. It now passes.I compared the outputs against an online eigenvalue calculator.
vmatch exactly andq_valuesare matching the results from the solver once normalized.I re-ran the code from #202 and it still doesn't return the expected values from the issue. I think there may be a problem in the code snippet provided too.
Summary by CodeRabbit