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

Test for chol!(Number) #24224

Closed
wants to merge 1 commit into from
Closed

Test for chol!(Number) #24224

wants to merge 1 commit into from

Conversation

kshyatt
Copy link
Contributor

@kshyatt kshyatt commented Oct 19, 2017

No description provided.

@kshyatt kshyatt added domain:linear algebra Linear algebra test This change adds or pertains to unit tests labels Oct 19, 2017
@@ -89,6 +89,9 @@ end
apos = apd[1,1] # test chol(x::Number), needs x>0
@test all(x -> x ≈ √apos, cholfact(apos).factors)
@test_throws PosDefException chol(-one(eltya))
# test chol! with a Number
chol_num = Base.LinAlg.chol!(apos, :U)
@test all(x -> x ≈ √apos, chol_num)
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps simply @test chol!(4, :U) ≈ 2? :)

@fredrikekre
Copy link
Member

fredrikekre commented Oct 20, 2017

Do we actually need chol!(x::Number)? it is a bit misleading with the !. I know we implement _chol!(x::Number, uplo) but the purpose of that is for the generic cholfact implementation to work correctly. I don't see chol!(x::Number) being used anywhere internally, and for users, chol(x) should do the exact same thing?

@mschauer
Copy link
Contributor

From

if eltya != Int #chol! won't work with Int
it appears that chol! it is/was used on numbers. In general, for chainable modifying array functions it is useful to treat immutables gracefully, e.g. in trace(chol!(B*B',:L)) is the point is to allow modification for efficiency, not to change B*B'.

@andreasnoack
Copy link
Member

@mschauer is right. I wanted to write a recursive Cholesky that didn't allocate. See

Akk, info = _chol!(A[k,k], LowerTriangular)
if info != 0
return LowerTriangular(A), info
end
A[k,k] = Akk
. For this to work, I had to define function that updated the in-place when possible. However, now that this is _chol!, there is probably no reason to have chol!.

@mschauer
Copy link
Contributor

Package authors appear to be a bit hesitant to provide methods for a method with the very internal name _chol! for their types, but that is exactly what they should do... maybe chol! can actually fill in here.

@andreasnoack
Copy link
Member

Why would they need to add methods to _chol!?

@mschauer
Copy link
Contributor

Because that is the function called on the elements of an array when a cholesky decomposition is performed.

@andreasnoack
Copy link
Member

Why are the existing methods not sufficient?

@kshyatt
Copy link
Contributor Author

kshyatt commented Oct 20, 2017

Did we come to a conclusion about whether to keep this or not?

@fredrikekre
Copy link
Member

I think so? @deprecate chol!(x::Number, uplo) chol(x)

@andreasnoack
Copy link
Member

Superseded by #24498

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:linear algebra Linear algebra test This change adds or pertains to unit tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants