Skip to content

Improve schur reliability#3634

Open
jacksonloper wants to merge 8 commits intojosdejong:developfrom
jacksonloper:copilot/improve-schur-reliability
Open

Improve schur reliability#3634
jacksonloper wants to merge 8 commits intojosdejong:developfrom
jacksonloper:copilot/improve-schur-reliability

Conversation

@jacksonloper
Copy link
Copy Markdown

@jacksonloper jacksonloper commented Jan 12, 2026

To fix #3600, shift strategy introduced to the Schur algorithm. Essentially this just takes the approach we already use to reliably get complex eigenvalues, and incorporates it here. (Well, not exactly the same approach; we use householders transforms here because need an orthogonal decomposition. But very similar!)

Copilot AI and others added 3 commits January 11, 2026 12:35
- Add Hessenberg reduction using Householder reflections
- Implement Francis double shift QR iteration for convergence
- Handle 2x2 blocks for complex conjugate eigenvalue pairs
- Add proper convergence detection and exceptional shifts
- Update tests to verify decomposition properties (A=UTU', orthogonality, quasi-triangularity)
- Add new tests for edge cases including rotation matrices with complex eigenvalues

Co-authored-by: jacksonloper <2056977+jacksonloper@users.noreply.github.com>
- 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>
@jacksonloper jacksonloper marked this pull request as ready for review January 27, 2026 16:37
@gwhitney
Copy link
Copy Markdown
Collaborator

gwhitney commented Mar 1, 2026

Sorry about the delay in looking at this -- I definitely have too many projects going on.

OK, there were definitely lots of problems that would prevent the code from working with BigNumber entries. I tried to get them out, but the only way to know if I succeeded is for you to add several tests for matrices with BigNumber entries. Please do so when you get a chance.

Also, I think that schur() is not supposed to handle matrices with complex entries. If that's correct, please check for complex entries and throw an error if you find them.

Similarly, should we expect schur() to handle rational (Fraction) entries? If so, please add tests for matrices with rational entries. If not, please check for them and throw an error.

Please check for bigint entries and convert to Fraction and run the algorithm, if it is expected to work with rationals, If it is not expected to work with rationals, throw an error on bigints, too.

Finally (at least for now), there's a bunch of blocks of similar code in schur.js at the moment. Please see if you can unify the code to avoid the near-duplication, which leads to maintenance problems down the line. In particular, the code applying the Householder reflections in both reduceToHessenberg and francisQR is quite similar. Then we also have computeHouseholderVector, computeHouseholderVector.2, and computeHouseholderVector3 -- can these be unified? I tried naively calling computeHouseholderVector in place of the _2 and _3 versions, but that caused the tests to fail. But maybe with a bit more care they can be unified.

I'll take another look when these things are done. Thanks so much for the contribution. I think some of it must have been written by AI, which doesn't mind code repetition, but it does make things painful to maintain.

@gwhitney
Copy link
Copy Markdown
Collaborator

gwhitney commented Mar 1, 2026

Oops, almost forgot -- the schur documentation needs one of the new History sections added. Please see the parse function for an example (or there are a bunch of other examples).

@jacksonloper
Copy link
Copy Markdown
Author

jacksonloper commented Mar 2, 2026

Those are all great questions! And yes there def is chatbot code in here :). I will dig in and

  • figure out how to make BigNumber work (or maybe it already does work b/c of your fixes), add tests
  • throw error for rational and bigint (in usual Schur Q has to be literally unitary (not scalar-multiple-of-unitary), so I don't see how rational could work)
  • decide whether we can handle complex and throw-or-handle
  • take a closer look at some of the nitty gritty, understand the maintainability issues, and sharpen my agent steering skills to address :).
  • understand histories sections and put one in

Jackson

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.

Schur result not quasi upper triangular

3 participants