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

Remove redundant uplo argument in chol family. #17909

Merged
merged 1 commit into from
Aug 10, 2016
Merged

Remove redundant uplo argument in chol family. #17909

merged 1 commit into from
Aug 10, 2016

Conversation

andreasnoack
Copy link
Member

When using Hermitian and Symmetric, it is redundant to also have an uplo argument and
occasionally, it also gave the wrong result. This can therefore be
considered a bugfix.

@kshyatt kshyatt added linear algebra Linear algebra bugfix This change fixes an existing bug needs docs Documentation for this change is required labels Aug 9, 2016
@kshyatt
Copy link
Contributor

kshyatt commented Aug 9, 2016

Would be nice to update the docstring about this as well.

@tkelman
Copy link
Contributor

tkelman commented Aug 9, 2016

new deprecations should not be backported. can you separate the bugfix from the deprecation into separate commits?

@andreasnoack
Copy link
Member Author

They should have been removed in #16799 but slipped through. Since they are wrong, it's unlikely that anybody relies on them so the right thing is to remove them (and they have only existed on master since June). My thinking was that the deprecation could be helpful if somebody accidentally called the methods but I can also just remove the methods without deprecation.

@@ -187,7 +197,7 @@ end

### for StridedMatrices, check that matrix is symmetric/Hermitian
"""
cholfact!(A, uplo::Symbol, Val{false}) -> Cholesky
cholfact!(A, [uplo::Symbol,] Val{false}) -> Cholesky
Copy link
Contributor

Choose a reason for hiding this comment

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

change this in the rst so the signatures match

@andreasnoack
Copy link
Member Author

Updated the doc string. Please comment.

@tkelman
Copy link
Contributor

tkelman commented Aug 9, 2016

I guess I'd prefer still doing a deprecation rather than a hard removal of the method, but if it's only been on master for a few months and is wrong, then we can put the deprecation in the 0.5 section and backport.

and Symmetric, it is redundant to also have an uplo argument and
occasionally, it also gave the wrong result. This can therefore be
considered a bugfix.
@tkelman tkelman merged commit 35df9b8 into master Aug 10, 2016
@tkelman tkelman deleted the anj/chol branch August 10, 2016 07:18
tkelman pushed a commit that referenced this pull request Aug 11, 2016
…17909)

and Symmetric, it is redundant to also have an uplo argument and
occasionally, it also gave the wrong result. This can therefore be
considered a bugfix.
(cherry picked from commit 35df9b8)
andreasnoack added a commit that referenced this pull request Aug 12, 2016
tkelman pushed a commit that referenced this pull request Aug 12, 2016
…mitian (#17909)"

This reverts commit d992f3d.

(cherry picked from commit 62b55ce)
ref #17985
mfasi pushed a commit to mfasi/julia that referenced this pull request Sep 5, 2016
…uliaLang#17909)

and Symmetric, it is redundant to also have an uplo argument and
occasionally, it also gave the wrong result. This can therefore be
considered a bugfix.
mfasi pushed a commit to mfasi/julia that referenced this pull request Sep 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This change fixes an existing bug linear algebra Linear algebra needs docs Documentation for this change is required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants