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

deprecate cholfact[!](A, uplo) #22188

Merged
merged 1 commit into from
Jun 5, 2017

Conversation

fredrikekre
Copy link
Member

Fix #22187

@@ -1350,6 +1350,14 @@ end
@deprecate srand(filename::AbstractString, n::Integer=4) srand(read!(filename, Array{UInt32}(Int(n))))
@deprecate MersenneTwister(filename::AbstractString) srand(MersenneTwister(0), read!(filename, Array{UInt32}(Int(4))))

# PR ??
Copy link
Contributor

Choose a reason for hiding this comment

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

would save some future work if you'd be willing to add in a news entry now

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure thing. Just didn't know what PR number this would end up with.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@deprecate cholfact(A::StridedMatrix, uplo::Symbol, ::Type{Val{false}}) cholfact(Hermitian(A, uplo), Val{false})
@deprecate cholfact(A::StridedMatrix, uplo::Symbol) cholfact(Hermitian(A, uplo))
@deprecate cholfact!(A::StridedMatrix, uplo::Symbol, ::Type{Val{true}}; tol = 0.0) cholfact!(Hermitian(A, uplo), Val{true}, tol = tol)
@deprecate cholfact(A::StridedMatrix, uplo::Symbol, ::Type{Val{true}}; tol = 0.0) cholfact(Hermitian(A, uplo), Val{true}, tol=tol)
Copy link
Member

Choose a reason for hiding this comment

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

Negligible, but inconsistent spacing around the keyword argument line to line? :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, fixed

@@ -94,18 +91,12 @@ using Base.LinAlg: BlasComplex, BlasFloat, BlasReal, QRPivoted, PosDefException
U = Bidiagonal([2,sqrt(eltya(3))],[-1],true) / sqrt(eltya(2))
@test full(chol(S)) ≈ full(U)

#lower Cholesky factor
Copy link
Member

Choose a reason for hiding this comment

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

Comment worth preserving/revising?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought it only applied to the tests which are now deprecated? Both Hermitian(A, :U) and Hermitian(A, :L) are tested in the tests just below.

Copy link
Member

Choose a reason for hiding this comment

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

My suggestion to preserve/revise the comment associated with that block was independent of whether that comment applies primarily or strictly to the deprecated tests: There is no other comment describing the surviving contents of that block, and having an up to date comment describing the remaining tests in that block would be worthwhile :).

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense! I will add something descriptive :)

Copy link
Member

Choose a reason for hiding this comment

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

Awesome! Thanks! :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done! Moved around some things, I think it is clearer now.

Copy link
Member

Choose a reason for hiding this comment

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

If you mean that the comment added on line 61 should also apply to this block, perhaps move this block adjacent to the block to which that comment is attached (rather than leave those blocks separated by the block dedicated to Strang matrices)? Forty lines, paragraph breaks, and other comments interceding obscures the relationship :).

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, I confused myself. Actually, there is no reason not to merge the two blocks, I will do that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ended up adding a comment instead.

@fredrikekre fredrikekre force-pushed the fe/cholesky-uplo branch 2 times, most recently from 5fc2d2d to 1ae54ff Compare June 2, 2017 21:19
@fredrikekre fredrikekre changed the title deprecate cholesky[!](A, uplo) deprecate cholfact[!](A, uplo) Jun 2, 2017
@Sacha0 Sacha0 added the deprecation This change introduces or involves a deprecation label Jun 4, 2017
restructure the cholesky test
@fredrikekre
Copy link
Member Author

Rebased.

Copy link
Member

@Sacha0 Sacha0 left a comment

Choose a reason for hiding this comment

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

Thanks @fredrikekre! :)

@KristofferC KristofferC merged commit c7b8eb9 into JuliaLang:master Jun 5, 2017
@fredrikekre fredrikekre deleted the fe/cholesky-uplo branch June 5, 2017 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecation This change introduces or involves a deprecation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants