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

Allow computing the correlation matrix from the covariance matrix only #661

Closed
wants to merge 4 commits into from
Closed

Conversation

ASaragga
Copy link

Following issue #652 in StatsBase.jl

Comment on lines +134 to +138
"""
cov2cor(C)

Compute the correlation matrix from the covariance matrix `C`. Use `StatsBase.cov2cor!` for an in-place version.
"""
Copy link
Member

Choose a reason for hiding this comment

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

I'd merge this with the docstring for the 2-argument version (with cov2cor(C[, s]) as a signature).

@nalimilan
Copy link
Member

Can you add tests?

@nalimilan
Copy link
Member

Did you remove the tests on purpose?

@ASaragga
Copy link
Author

ASaragga commented Feb 23, 2021 via email

@JockLawrie
Copy link

Hi there,

Looks like the method hasn't been implemented.
I can't edit this PR. Can someone add the following code? (and is it sufficient?)
Alternatively should I submit a separate PR?

In src/cov.jl

cov2cor!(C) = cov2cor!(C, [sqrt(C[i,i]) for i = 1:size(C, 1)])
cov2cor(C) = cov2cor!(copy(C))

In test/cov.jl

# Around line 126
@test cov2cor(cov1)  cor1
@test cov2cor(cov2)  cor2

# Around line 202
@test cov2cor(cov1)  cor1
@test cov2cor(cov2)  cor2

# Around line 209
StatsBase.cov2cor!(tmp_cov1)
@test tmp_cov1  cor1

# Around line 214
StatsBase.cov2cor!(tmp_cov2)
@test tmp_cov2  cor2

@nalimilan
Copy link
Member

Please make a new PR, that's simpler.

@andreasnoack
Copy link
Member

Superseded by #816

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants