Skip to content

Commit 904c21e

Browse files
Copilotjacksonloper
andcommitted
Address code review feedback
- Simplify Householder sigma check condition - Clarify discriminant comment for eigenvalue classification - Fix potential undefined access in exceptional shift calculation - Update docstring example to describe properties rather than specific values - Make 2x2 block test threshold relative to matrix norm Co-authored-by: jacksonloper <2056977+jacksonloper@users.noreply.github.com>
1 parent cff4945 commit 904c21e

File tree

2 files changed

+16
-10
lines changed

2 files changed

+16
-10
lines changed

src/function/algebra/decomposition/schur.js

Lines changed: 13 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -55,8 +55,11 @@ export const createSchur = /* #__PURE__ */ factory(name, dependencies, (
5555
*
5656
* Examples:
5757
*
58-
* const A = [[1, 0], [-4, 3]]
59-
* math.schur(A) // returns {U: [[0, 1], [-1, 0]], T: [[3, 4], [0, 1]]}
58+
* const A = [[1, 2], [0, 3]]
59+
* const result = math.schur(A)
60+
* // result.U is orthogonal: U * U' = I
61+
* // result.T is upper triangular (quasi-upper-triangular)
62+
* // A = U * T * U'
6063
*
6164
* See also:
6265
*
@@ -210,10 +213,7 @@ export const createSchur = /* #__PURE__ */ factory(name, dependencies, (
210213
const x0 = x[0]
211214
const x0sq = multiplyScalar(x0, x0)
212215

213-
// If the vector is already a multiple of e_1, no transformation needed
214-
if (abs(sigma) < 1e-14 && abs(x0) < 1e-14) {
215-
return null
216-
}
216+
// If the vector is already a multiple of e_1 (sigma ≈ 0), no transformation needed
217217
if (abs(sigma) < 1e-14) {
218218
return null
219219
}
@@ -292,7 +292,9 @@ export const createSchur = /* #__PURE__ */ factory(name, dependencies, (
292292
const c = H[p][p - 1]
293293
const d = H[p][p]
294294

295-
// Discriminant: (a-d)^2 + 4bc (derived from characteristic polynomial)
295+
// Discriminant of characteristic polynomial λ² - (a+d)λ + (ad-bc)
296+
// discriminant = (a+d)² - 4(ad-bc) = (a-d)² + 4bc
297+
// If discriminant < 0, eigenvalues are complex conjugates
296298
const diff = subtractScalar(a, d)
297299
const discriminant = addScalar(multiplyScalar(diff, diff), multiplyScalar(4, multiplyScalar(b, c)))
298300

@@ -317,8 +319,10 @@ export const createSchur = /* #__PURE__ */ factory(name, dependencies, (
317319

318320
// Apply exceptional shift if convergence is slow
319321
if (iterCount === 10 || iterCount === 20) {
320-
// Exceptional shift: use a random-ish perturbation
321-
const shift = multiplyScalar(addScalar(abs(H[p][p - 1]), abs(H[p - 1][p - 2] || 0)), iterCount === 10 ? 1.5 : -1.5)
322+
// Exceptional shift: use a random-ish perturbation based on subdiagonal elements
323+
const subdiagVal = abs(H[p][p - 1])
324+
const prevSubdiag = (p >= 2) ? abs(H[p - 1][p - 2]) : 0
325+
const shift = multiplyScalar(addScalar(subdiagVal, prevSubdiag), iterCount === 10 ? 1.5 : -1.5)
322326
for (let i = q; i <= p; i++) {
323327
H[i][i] = addScalar(H[i][i], shift)
324328
}

test/unit-tests/function/algebra/decomposition/schur.test.js

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -116,7 +116,9 @@ describe('schur', function () {
116116
assert.ok(Math.abs(T[1][0]) < 1e-10, 'T[1][0] should be zero')
117117
assert.ok(Math.abs(T[2][0]) < 1e-10, 'T[2][0] should be zero')
118118
// T[2][1] should be non-zero (complex eigenvalue 2x2 block)
119-
assert.ok(Math.abs(T[2][1]) > 0.1, 'T[2][1] should be non-zero for complex eigenvalue block')
119+
// The magnitude should be significant relative to the matrix norm
120+
const matrixNorm = math.norm(A)
121+
assert.ok(Math.abs(T[2][1]) > 1e-10 * matrixNorm, 'T[2][1] should be non-zero for complex eigenvalue block')
120122
})
121123

122124
it('should handle symmetric matrix', function () {

0 commit comments

Comments
 (0)