Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

patches to matrix log #33245

Merged
merged 2 commits into from
Sep 20, 2019
Merged

patches to matrix log #33245

merged 2 commits into from
Sep 20, 2019

Conversation

RalphAS
Copy link

@RalphAS RalphAS commented Sep 13, 2019

Avoid integer overflow if s > 63.
Correct logic for s == 0.
Only use fancy divided difference formulae if eigenvalues are close - avoids dangerous roundoff error if they are in opposite sectors.

Avoid integer overflow if `s > 63`.
Correct logic for `s == 0`.
Only use fancy divided difference formulae if eigenvalues are close - avoids dangerous roundoff error if they are in opposite sectors.
@KristofferC KristofferC added the needs tests Unit tests are required for this change label Sep 13, 2019
@andreasnoack andreasnoack added the linear algebra Linear algebra label Sep 13, 2019
@andreasnoack
Copy link
Member

@RalphAS Thanks for fixing this. Would you be able to add some tests that would have failed before the changes?

@RalphAS
Copy link
Author

RalphAS commented Sep 14, 2019

@andreasnoack In case it's not clear, this patch cleans up some loose ends in PR #32327 (and is a PR to the associated branch); in particular it fixes a regression observed by @stevengj there.

I can try to generate some manageable tests for the other fixes - it's not trivial because the algorithm manipulates the matrix a lot before it gets to the sections patched here.
(Edit: easier than I thought.)

I have a project including a larger test suite (with less manageable matrices) that I used for this problem, and I intend to put that on Github soon.

@stevengj
Copy link
Member

LGTM.

@andreasnoack andreasnoack merged commit db1cd35 into JuliaLang:sgj/logfix Sep 20, 2019
andreasnoack pushed a commit that referenced this pull request Sep 23, 2019
* bug fixes in matrix log

* patches to matrix log (#33245)

* patches to matrix log

Avoid integer overflow if `s > 63`.
Correct logic for `s == 0`.
Only use fancy divided difference formulae if eigenvalues are close - avoids dangerous roundoff error if they are in opposite sectors.

* add tests
KristofferC pushed a commit that referenced this pull request Dec 3, 2019
* bug fixes in matrix log

* patches to matrix log (#33245)

* patches to matrix log

Avoid integer overflow if `s > 63`.
Correct logic for `s == 0`.
Only use fancy divided difference formulae if eigenvalues are close - avoids dangerous roundoff error if they are in opposite sectors.

* add tests

(cherry picked from commit 318affa)
KristofferC pushed a commit that referenced this pull request Feb 20, 2020
* bug fixes in matrix log

* patches to matrix log (#33245)

* patches to matrix log

Avoid integer overflow if `s > 63`.
Correct logic for `s == 0`.
Only use fancy divided difference formulae if eigenvalues are close - avoids dangerous roundoff error if they are in opposite sectors.

* add tests

(cherry picked from commit 318affa)
KristofferC pushed a commit that referenced this pull request Feb 20, 2020
* bug fixes in matrix log

* patches to matrix log (#33245)

* patches to matrix log

Avoid integer overflow if `s > 63`.
Correct logic for `s == 0`.
Only use fancy divided difference formulae if eigenvalues are close - avoids dangerous roundoff error if they are in opposite sectors.

* add tests

(cherry picked from commit 318affa)
KristofferC pushed a commit that referenced this pull request Feb 22, 2020
* bug fixes in matrix log

* patches to matrix log (#33245)

* patches to matrix log

Avoid integer overflow if `s > 63`.
Correct logic for `s == 0`.
Only use fancy divided difference formulae if eigenvalues are close - avoids dangerous roundoff error if they are in opposite sectors.

* add tests

(cherry picked from commit 318affa)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
linear algebra Linear algebra needs tests Unit tests are required for this change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants