Scale Corr

[1e-to]

[pep8speaks]

Hello @1e-to! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-02-20 11:14:30 UTC

[PokhodenkoSA]

Duplicate function definition. Other definition in line 507.

[AlexanderKalistratov]

corr(x,y) = cov(x, y)/sqrt(var(x)*var(y))

Please note, that cov here is unbiased, while Series.cov calculates biased one

Biasedcov calculation is here:
covariance

Suggestion for var implementation is here:
variance

Considering all of it, corr calculation would be something like this:

defhpat_pandas_series_corr_impl(self, other, min_periods=None):
ifmin_periodsisNoneormin_periods<1:
min_periods=1min_len=min(len(self._data), len(other._data))
ifmin_len==0:
returnnumpy.nansum_y=0.sum_x=0.sum_xy=0.sum_xx=0.sum_yy=0.total_count=0foriinprange(min_len):
x=self._data[i]
y=other._data[i]
ifnot (numpy.isnan(x) ornumpy.isnan(y)):
sum_x+=xsum_y+=ysum_xy+=x*ysum_xx+=x*xsum_yy+=y*ytotal_count+=1iftotal_count<min_periods:
returnnumpy.nancov_xy= (sum_xy-sum_x*sum_y/total_count)
var_x= (sum_xx-sum_x*sum_x/total_count)
var_y= (sum_yy-sum_y*sum_y/total_count)
corr_xy=cov_xy/sqrt(var_x*var_y)
returncorr_xy

Haven't verified it. So, please, double check it and fix any mistakes in calculation.

Also, if we had loops fusion, we'd be able to simply call three functions (cov(x,y), var(x) and var(y)).
But I don't think that at the moment it is possible

[1e-to]

Also, if we had loops fusion, we'd be able to simply call three functions (cov(x,y), var(x) and var(y)).
But I don't think that at the moment it is possible

I tried to use our algorithms for var and cov to test fusing with Todd’s fix, but the unit tests stopped passing, the algorithm does not work correctly. Results have not needed accurancy. Does this mean that we need to rewrite the var and cov?
And also, what is the difference between biased from unbiased?

[AlexanderKalistratov]

Also, if we had loops fusion, we'd be able to simply call three functions (cov(x,y), var(x) and var(y)).
But I don't think that at the moment it is possible

I tried to use our algorithms for var and cov to test fusing with Todd’s fix, but the unit tests stopped passing, the algorithm does not work correctly. Results have not needed accurancy. Does this mean that we need to rewrite the var and cov?
And also, what is the difference between biased from unbiased?

It doesn't match because Series.conv calculates biased cov. And you need unbiased one.
Also, there could be a problem with nans. When calculating var we need to skip values if either of two is nan. Series.var skips only nans in it.

Have you tried the provided solution?