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

add issuccess for CholeskyPivoted #36002

Merged
merged 2 commits into from
May 24, 2020
Merged

add issuccess for CholeskyPivoted #36002

merged 2 commits into from
May 24, 2020

Conversation

palday
Copy link
Contributor

@palday palday commented May 23, 2020

Trivial change to add issuccess method for CholeskyPivoted. Let me know if (and where!) I need to add an additional test to cover this.

palday added a commit to JuliaStats/MixedModels.jl that referenced this pull request May 23, 2020
@ViralBShah ViralBShah added the linear algebra Linear algebra label May 23, 2020
@ViralBShah
Copy link
Member

The test goes in: stdlib/LinearAlgebra/test/cholesky.jl

@ararslan ararslan added needs compat annotation Add !!! compat "Julia x.y" to the docstring needs news A NEWS entry is required for this change needs tests Unit tests are required for this change labels May 23, 2020
@ararslan
Copy link
Member

ararslan commented May 23, 2020

Hey Phillip, welcome and thanks for the contribution! Some things to add here:

  • At the bottom of the issuccess docstring, something like
    !!! compat "Julia 1.6"
        `issuccess(::CholeskyPivoted)` requires Julia 1.6 or later.
    
  • An entry in the LinearAlgebra section of NEWS.md that mentions the addition of this method
  • A test for this method in the location Viral pointed out

This functionality looks like a very useful addition!

@palday
Copy link
Contributor Author

palday commented May 24, 2020

Okay, I think I got everything taken care of. Let me know what I missed. :)

@ViralBShah ViralBShah merged commit 1747eac into JuliaLang:master May 24, 2020
@ViralBShah
Copy link
Member

Thanks @palday and welcome!

@ViralBShah ViralBShah removed needs compat annotation Add !!! compat "Julia x.y" to the docstring needs news A NEWS entry is required for this change needs tests Unit tests are required for this change labels May 24, 2020
@palday palday deleted the pa/issuccess branch May 24, 2020 21:45
@andreasnoack
Copy link
Member

We discussed this and the possible issues in JuliaLang/LinearAlgebra.jl#720

@ViralBShah
Copy link
Member

Anything we should do differently here. Sorry, I wasn't aware of JuliaLang/LinearAlgebra.jl#720.

@andreasnoack
Copy link
Member

I think we concluded the definition here was reasonable even though it was argued that the factorization can also be successful when info > 0 and, in fact, that it will always be successful for positive semidefinite matrices. However, it would be good to make these caveats clear in the docs.

@ViralBShah
Copy link
Member

@palday Would it be possible for you to review JuliaLang/LinearAlgebra.jl#720 and update the docs accordingly?

@palday
Copy link
Contributor Author

palday commented May 25, 2020

I also gave some thought to the successful rank-deficient PSD case, but in check=true you would get a RankDeficientException, so the behavior is self-consistent if unfortunately returning false negatives for this case.

@palday
Copy link
Contributor Author

palday commented May 25, 2020

Ha, it looks like that issue is discussing exactly what I discovered. I'll take a look (might be a few days).

simeonschaub pushed a commit to simeonschaub/julia that referenced this pull request Aug 11, 2020
* add issuccess for CholeskyPivoted

* add tests, NEWS and compat
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
linear algebra Linear algebra
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants