fix: BigNumber eigenvalue computation broken by Decimal.js string comparison#3655
fix: BigNumber eigenvalue computation broken by Decimal.js string comparison#3655danielsimonjr wants to merge 1 commit intojosdejong:developfrom
Conversation
… computation
Decimal.js's valueOf() returns a string, so JavaScript's comparison
operators (>=, <, <=) perform lexicographic comparison instead of
numeric when comparing BigNumber values. For example:
new Decimal(1) >= new Decimal('5e-13') // false ('1' < '5' in ASCII)
new Decimal(1).gte(new Decimal('5e-13')) // true (correct)
This caused the Jacobi rotation convergence loop in diagBig() to never
execute for BigNumber inputs — eigenvalues were returned as the
unchanged diagonal entries instead of being computed.
The fix uses Decimal's native .gte()/.lt()/.lte() methods wrapped in
helper functions that fall through to native operators for plain numbers.
Also fixes:
- getAijBig: initialize maxMij as bignumber(0) instead of plain 0
- diagBig: initialize identity matrix with bignumber values
- diagBig: remove unnecessary Array.fill(0) for eigenvalue array
- getThetaBig: wrap config.relTol in bignumber() for comparison
Fixes eigenvalue computation for all BigNumber matrix inputs.
There was a problem hiding this comment.
Pull request overview
Fixes incorrect BigNumber (Decimal.js) eigenvalue computation in the real-symmetric Jacobi diagonalization path by replacing lexicographic JS relational comparisons with numeric BigNumber comparisons, and improving BigNumber type consistency in key initialization sites.
Changes:
- Add BigNumber-safe comparison helpers (
bigGte,bigLt,bigLte) and use them in convergence, pivot selection, and eigenvalue sorting. - Make BigNumber execution paths more type-consistent (BigNumber identity matrix initialization;
maxMijinitialized asbignumber(0)). - Ensure tolerance comparison in
getThetaBigis performed in BigNumber space.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| function diagBig (x, precision, computeVectors) { | ||
| const N = x.length | ||
| const e0 = abs(precision / N) | ||
| const e0 = abs(bignumber(precision) / N) |
There was a problem hiding this comment.
bignumber(precision) / N uses JavaScript's / operator, which will coerce the BigNumber (Decimal.js) to a primitive and perform a native number division. This can silently lose BigNumber precision (and can underflow to 0 for very small BigNumber precisions), affecting the convergence threshold e0.
Use BigNumber-safe arithmetic here (e.g., bignumber(precision).div(N) or an equivalent using multiply/inv/multiplyScalar) so e0 stays a BigNumber and preserves the requested precision.
| const e0 = abs(bignumber(precision) / N) | |
| const e0 = abs(multiplyScalar(bignumber(precision), 1 / N)) |
|
@danielsimonjr thanks for your PR. I'm a bit cautious here, I don't want to end up in a loop with two AI's, so I didn't review this PR indepth. I'm not sure why Copilot left a comment here, did you request a copilot review? Feedback:
|
Summary
BigNumber eigenvalue computation (
eigs()) silently returns incorrect results for all BigNumber matrix inputs. The Jacobi rotation convergence loop never executes because JavaScript's comparison operators perform lexicographic string comparison on Decimal.js objects instead of numeric comparison.Root Cause
Decimal.js'svalueOf()returns a string, not a number. When JavaScript's>=,<,<=operators compare two objects, they callvalueOf()on both operands. With strings, comparison is lexicographic:This affects 4 comparison sites in
realSymmetric.js:diagBigconvergence loop:abs(Vab[1]) >= abs(e0)— loop never executesgetThetaBigthreshold:abs(denom) <= config.relTol— wrong branch takengetAijBigmax finding:abs(maxMij) < abs(Mij[i][j])— wrong max selectedsortingeigenvalue ordering:abs(E[j]) < abs(minE)— wrong sort orderFix
Added helper functions that use Decimal.js's native comparison methods (
.gte(),.lt(),.lte()) which perform correct numeric comparison. The helpers fall through to native operators for plainnumbertypes.Also fixes:
getAijBig: initializemaxMijasbignumber(0)instead of plain0diagBig: initialize identity matrix withbignumber()values instead of plain numbersgetThetaBig: wrapconfig.relTolinbignumber()for type-safe comparisonTest Plan
eigs([[0,1],[1,0]])returns eigenvalues{-1, 1}(was returning{0, 0})eigs([[1,1],[1,1]])returns eigenvalues{0, 2}(was returning{1, 1})Impact
This bug affects all BigNumber eigenvalue computations. Any user calling
eigs()with BigNumber matrix inputs gets incorrect results (the unchanged diagonal entries instead of actual eigenvalues). Number-type inputs are unaffected.🤖 Generated with Claude Code