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

Remove single-argument _chol!(A::StridedMatrix) in cholesky.jl #34850

Merged
merged 2 commits into from
Feb 29, 2020
Merged

Remove single-argument _chol!(A::StridedMatrix) in cholesky.jl #34850

merged 2 commits into from
Feb 29, 2020

Conversation

ShriprajwalK
Copy link
Contributor

No description provided.

andreasnoack
andreasnoack reviewed Feb 24, 2020
View reviewed changes
Copy link
Member

@andreasnoack andreasnoack left a comment

Choose a reason for hiding this comment

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

Did you check test coverage? I think this function is already covered by existing tests.

stdlib/LinearAlgebra/test/cholesky.jl Outdated Show resolved Hide resolved
@ShriprajwalK
Copy link
Contributor Author

Did you check test coverage? I think this function is already covered by existing tests.

I did check coveralls. This function is still in red.
https://coveralls.io/builds/28940782/source?filename=stdlib/LinearAlgebra/src/cholesky.jl
This is testing the function in line 151-156 which is in red. It is the third method of _chol!()

@kshyatt kshyatt added domain:linear algebra Linear algebra test This change adds or pertains to unit tests labels Feb 25, 2020
@andreasnoack
Copy link
Member

Just took a closer look. I think this function is dead code at this point. We used to have a chol! function which called it but it's gone. @fredrikekre would you be able to verify that it's no longer used anywhere?

@fredrikekre fredrikekre self-requested a review February 25, 2020 23:42
@ShriprajwalK
Copy link
Contributor Author

Oh. So this method for the function should be deleted right?

@dkarrasch
Copy link
Member

AFAICT, there is no single-argument call of _chol! anywhere. All calls pass a LowerTriangular or UpperTriangular type argument as second argument.

@dkarrasch dkarrasch changed the title Added tests for _chol!(A::StridedMatrix) in cholesky.jl Remove single-argument _chol!(A::StridedMatrix) in cholesky.jl Feb 26, 2020
@dkarrasch dkarrasch removed the test This change adds or pertains to unit tests label Feb 26, 2020
@dkarrasch
Copy link
Member

This looks pretty good. Is there anything we need to take into account? Since this is not exported, I guess we don't need to consider external packages depending on this. Can we merge then?

@ShriprajwalK
Copy link
Contributor Author

Good to go from my side

@fredrikekre fredrikekre merged commit e39e945 into JuliaLang:master Feb 29, 2020
ravibitsgoa pushed a commit to ravibitsgoa/julia that referenced this pull request Apr 9, 2020
@ShriprajwalK @ravibitsgoa
Remove unused single-argument _chol!(A::StridedMatrix) in cholesky.jl (…
93a2ec1 
…JuliaLang#34850)
KristofferC pushed a commit that referenced this pull request Apr 11, 2020
@ShriprajwalK @KristofferC
Remove unused single-argument _chol!(A::StridedMatrix) in cholesky.jl (…
ce56870 
…#34850)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vtjnash vtjnash vtjnash left review comments

@andreasnoack andreasnoack andreasnoack left review comments

@fredrikekre fredrikekre fredrikekre approved these changes

Assignees
No one assigned
Labels
domain:linear algebra Linear algebra
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@ShriprajwalK @andreasnoack @dkarrasch @vtjnash @fredrikekre @kshyatt