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 passing 0-length vectors to cor #70

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bkamins
Copy link
Contributor

@bkamins bkamins commented Jan 23, 2021

I have not added the tests yet. Let us first decide if we like the design of what I propose and then I will finalize the PR.

The proposed change is non-breaking.

@codecov
Copy link

codecov bot commented Jan 23, 2021

Codecov Report

Merging #70 (146bde2) into master (7b56a27) will decrease coverage by 0.99%.
The diff coverage is 20.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #70      +/-   ##
==========================================
- Coverage   97.41%   96.41%   -1.00%     
==========================================
  Files           1        1              
  Lines         387      391       +4     
==========================================
  Hits          377      377              
- Misses         10       14       +4     
Impacted Files Coverage Δ
src/Statistics.jl 96.41% <20.00%> (-1.00%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7b56a27...146bde2. Read the comment docs.

@@ -685,7 +685,14 @@ function corm(x::AbstractVector, mx, y::AbstractVector, my)
require_one_based_indexing(x, y)
n = length(x)
length(y) == n || throw(DimensionMismatch("inconsistent lengths"))
n > 0 || throw(ArgumentError("correlation only defined for non-empty vectors"))
if n == 0
T = promote_type(typeof(mx), typeof(my))
Copy link
Member

@nalimilan nalimilan Jan 23, 2021

Choose a reason for hiding this comment

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

How about using an equation closer to the actual computation of the correlation, like the one used below with zero?

EDIT: though it's true that at JuliaLang/julia#29033 we used oftype(..., NaN).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did this this way as x and y can have abstract eltype (e.g. Any), in which case you cannot use zero on it.

This code assumes that type of mx and my is somehow "derived" from x and y. As corm is unexported I thought it should be safe.

But we might use something like: sqrt(zero(mx)*zero(my)/(abs2(one(mx)*abs2(one(my)))

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that formula looks good (with zero at the denominator)!

@bkamins
Copy link
Contributor Author

bkamins commented Jan 23, 2021

Also probably cov should be fixed (I will try to look into it later, after I am done with DataFrames.jl reviews).

@nalimilan
Copy link
Member

+1

Relevant previous discussions:

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.

2 participants