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

Cholesky: Fix misleading error message #189

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

vks
Copy link

@vks vks commented Jun 2, 2017

The diagonals of the decomposed matrix were negative, which means the original matrix was not positive definite (even if the diagonal was positive).

The diagonals of the *demoposed* matrix were negative, which means the original matrix was not positive definite (even the the diagonal was positive).
@AtheMathmo
Copy link
Owner

I think this is a good change from the user perspective! @Andlon am I missing anything here?

@Andlon
Copy link
Collaborator

Andlon commented Jun 3, 2017

Thanks for the PR @vks! Yes, I agree that the original error message is indeed misleading.

Note that the documentation for the method also needs to be updated as it has a similar wording as the original error message. Would you mind changing this as well @vks? In this case, we need to be a bit careful with how it is worded.

I see now that also the other documented error message is misleading, since it states that:

A diagonal entry is effectively zero to working precision.

As you pointed out, it is not the diagonal entry in the original matrix, but the diagonal entry in the decomposition which is being carried out. Perhaps just replace this by "The matrix is effectively singular to working precision"?

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.

3 participants