Allow omitting standard deviation argument to cov2cor

[ASaragga]

The function cov2cor(C,s) source code states that: '[It] compute[s] the correlation matrix from the covariance matrix C and a vector of standard deviations s. I was expecting a function that would compute the correlation matrix AND the standard deviation, from the covariance matrix only. This approach is followed by Matlab.

[nalimilan]

Yes our function differs from MATLAB's cov2corr. Actually our method is equivalent to R's cov2cor. Anyway the docstring seems explicit enough, and the name also matches what it does quite well.

If you want the standard deviations then you can just extract the diagonal of the covariance matrix. Maybe we need a convenience function to do that (see JuliaLang/julia#30250), but I don't think we should make cov2cor return the standard deviations as it would be a waste when you don't want them.

[ASaragga]

Well, my issue was that we don't need the vector of standard deviations sto compute the correlation matrix from the covariance matrix. In my opinion, it would be better to have just cov2cor(C), instead of cov2cor(C,s).

[nalimilan]

Ah,good point.

@rofinn @ararslan @andreasnoack Any reason why when cor2cov was added at #261 the vector of standard deviations s isn't computed automatically from the covariance matrix if not provided?

[ararslan]

No idea on my end. I have little to no recollection of that PR. Naively, it seems like a reasonable thing to do.

[ASaragga]

R and Matlab both have the same signature, without a standard deviation argument:

• Matlab: R = corrcov(C) returns the correlation matrix R corresponding to the covariance matrix C.

• R: cov2cor(V) convert a covariance matrix to a correlation matrix.

Find it rather puzzling why one is asked to provide the standard deviation in this case.

[nalimilan]

I suspect the reason is historical: this function was originally used internally in Base (now Statistics) to compute the correlation, by a function which had already computed the standard deviations.

[rofinn]

Yeah, I think we were just trying to minimally mimic base. Since I was already using a custom weighted std, it didn't cross my mind to add that default. Seems like a good thing to add though 👍🏻

[nalimilan]

OK, so if anybody wants to make a PR... Should be quite easy.

[ASaragga]

I may try to make a PR... Although I am very inexperienced concerning PRs. It is a little embarrassing, but I couldn't find the function cov2cor! that is called by cov2cor, except in /test/cov.jl (although cor2cov!is in the file /src/cov.jl).

[mschauer]

We discussed is once in one of the chat and also agreed that this would be nice to have.

[nalimilan]

I may try to make a PR... Although I am very inexperienced concerning PRs. It is a little embarrassing, but I couldn't find the function cov2cor! that is called by cov2cor, except in /test/cov.jl (although cor2cov!is in the file /src/cov.jl).

cov2cor! is actually defined in Statistics.jl. This is due to history as noted above. It would make sense to export both cov2cor and cov2cor! from Statistics I guess.

[ASaragga]

Made minimal changes to Statistics.jl and StatsBase.jl to allow computing the the correlation matrix from the covariance matrix only. Those changes are in ASaragga/Statistics.jl and ASaragga/StatsBase.jl, respectively. Are they OK to make a PR?

[nalimilan]

You don't actually need to duplicate the full definition of the method AFAICT: just define a default value like this

cov2cor!(C::AbstractMatrix{T}, xsd::AbstractArray=[sqrt(C[i,i]) for i in 1:size(C, 1)])

[ararslan]

Or sqrt.(diag(C))

[nalimilan]

Yeah but that makes a copy.

[ararslan]

sqrt.(view(C, diagind(C)))? 😄