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.

Sign up for GitHub

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

Jump to bottom

Use exact algorithm for cond(A,1), and fix for singular matrices #33548

Merged
merged 7 commits into from
Oct 17, 2019
Merged

Use exact algorithm for cond(A,1), and fix for singular matrices #33548

merged 7 commits into from
Oct 17, 2019

Conversation

antoine-levitt
Copy link
Contributor

Fix #33297
Fix #33547

@dkarrasch dkarrasch added the domain:linear algebra Linear algebra label Oct 13, 2019
goggle
goggle suggested changes Oct 13, 2019
View reviewed changes
stdlib/LinearAlgebra/src/dense.jl Outdated Show resolved Hide resolved
stdlib/LinearAlgebra/src/dense.jl Outdated Show resolved Hide resolved
stdlib/LinearAlgebra/test/lu.jl Outdated Show resolved Hide resolved
@antoine-levitt
Copy link
Contributor Author

Thanks @goggle, I changed according to your input. I used the same (convoluted) logic as in inv for computing the output type.

@dkarrasch
Copy link
Member

Looks good to me, let's try to have another pair of eyes on it. BTW, do we need to announce somewhere that we're no longer (silently) returning condition number estimates in some cases? This certainly changes behavior, now the question is if that's simply a "bugfix" or a "breaking change"?

@StefanKarpinski StefanKarpinski added the kind:minor change Marginal behavior change acceptable for a minor release label Oct 15, 2019
@StefanKarpinski
Copy link
Sponsor Member

Changing behavior is technically breaking, so I'm putting the "minor change" label on this, but going from an estimate to an exact error is certainly an improvement, so permissible in a minor version. Should have a NEWS entry, however.

@StefanKarpinski StefanKarpinski added the needs news A NEWS entry is required for this change label Oct 15, 2019
@antoine-levitt
Copy link
Contributor Author

Done

@dkarrasch dkarrasch removed the needs news A NEWS entry is required for this change label Oct 17, 2019
@StefanKarpinski
Copy link
Sponsor Member

This seems to be good to go as soon as CI passes.

@StefanKarpinski StefanKarpinski merged commit e42a2a8 into JuliaLang:master Oct 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@goggle goggle goggle approved these changes

@dkarrasch dkarrasch dkarrasch approved these changes

@andreasnoack andreasnoack Awaiting requested review from andreasnoack

Assignees
No one assigned
Labels
domain:linear algebra Linear algebra kind:minor change Marginal behavior change acceptable for a minor release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Condition number is computed wrong in some cases cond(A,1) fails to produce Inf for A exactly singular
4 participants
@antoine-levitt @dkarrasch @StefanKarpinski @goggle