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

Fix for one tiny aspect of the rank deficiency issues #367

Merged
merged 2 commits into from
Sep 10, 2020
Merged

Conversation

palday
Copy link
Member

@palday palday commented Sep 9, 2020

This fixes one tiny facet of the bigger issue lurking in #324.

In particular, OpenBLAS on SkyLakeX can do something weird for some (full rank!) matrices:

  • The pivoted Cholesky estimates rank below full.
  • The unpivoted Cholesky has info ==0, which means that the unpivoted Cholesky succeeded, suggesting numerical full rank.

In this case, the old code generated an invalid permutation consisting of the identity permutation with an extra element 0 appended to the end. The fix here trusts the success of the unpivoted Cholesky more than the rank of the pivoted Cholesky.

@palday palday requested a review from dmbates September 9, 2020 19:42
Copy link
Collaborator

@dmbates dmbates left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Let's wait for the checks to run then feel free to merge.

@codecov
Copy link

codecov bot commented Sep 9, 2020

Codecov Report

Merging #367 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #367   +/-   ##
=======================================
  Coverage   95.40%   95.40%           
=======================================
  Files          23       23           
  Lines        1503     1503           
=======================================
  Hits         1434     1434           
  Misses         69       69           
Impacted Files Coverage Δ
src/linalg/statschol.jl 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9b7a748...d3f85e2. Read the comment docs.

@dmbates
Copy link
Collaborator

dmbates commented Sep 9, 2020

Is the failure on 1.5 a problem? The failure on nightly is because Showoff hasn't been updated for the Grisu -> Ryu that took place back in the Julia-1.4.0 release.

@palday
Copy link
Member Author

palday commented Sep 9, 2020

The failure for 1.5 is the main rank deficiency issue (#324) and the failure for nightly is the Showoff issue (#363).

@palday
Copy link
Member Author

palday commented Sep 9, 2020

It's not much comfort, but on my other Linux machine without SkyLakeX, the tests pass on 1.5.

(I have learned far too much about numerical linear algebra and its interaction with different SIMD instructions for this issue ... and yet I still know that I know nothing.)

@palday palday merged commit 7c0f9e1 into master Sep 10, 2020
@dmbates
Copy link
Collaborator

dmbates commented Sep 10, 2020

It's not much comfort, but on my other Linux machine without SkyLakeX, the tests pass on 1.5.

(I have learned far too much about numerical linear algebra and its interaction with different SIMD instructions for this issue ... and yet I still know that I know nothing.)

One of Piet Hein's "grooks" is

Knowing what thou knowest not is, in a sense, omniscience.

@palday palday deleted the pa/babycholesky branch November 19, 2020 17:16
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.

2 participants