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

Corr comp #159

Merged
merged 12 commits into from Jan 13, 2017
Merged

Corr comp #159

merged 12 commits into from Jan 13, 2017

Conversation

@yidawang
Copy link
Member

@yidawang yidawang commented Jan 13, 2017

Add a high-performance correlation computation function using BLAS wrapped in Cython to brainiak/fcma/util. Running at least one order of magnitude faster than np.corrcoef.

Ours: 100 loops, best of 3: 14.2 ms per loop
np.corrcoef: 10 loops, best of 3: 153 ms per loop
Correlate the rows of set1 with the rows of set2.
If set1 == set2, it is auto-correlation computation
resulting in a symmetric correlation matrix.
The number of columns MUST agree between set1 and set2

This comment has been minimized.

@mihaic

mihaic Jan 13, 2017
Contributor

Could you please add a reference for the method of correlation computation you are using? Some Latex code could help too.

This comment has been minimized.

@yidawang

yidawang Jan 13, 2017
Author Member

done

return data


def compute_correlation(set1, set2):

This comment has been minimized.

@mihaic

mihaic Jan 13, 2017
Contributor

Shouldn't the parameters be called array1/2 or matrix1/2?

This comment has been minimized.

@yidawang

yidawang Jan 13, 2017
Author Member

they are essentially two matrices, depicting two sets of variables. I can use matrix1/2

yidawang added 3 commits Jan 13, 2017
@@ -70,6 +70,19 @@ def compute_correlation(matrix1, matrix2):
is the standard deviation of variable X
Reducing the correlation computation to matrix multiplication
and use BLAS GEMM API wrapped by Scipy can speedup the numpy build-in

This comment has been minimized.

@mihaic

mihaic Jan 13, 2017
Contributor

use->using, build-in->built-in

This comment has been minimized.

@yidawang

yidawang Jan 13, 2017
Author Member

done

import math


def normalize_for_correlation(data, axis):

This comment has been minimized.

@mihaic

mihaic Jan 13, 2017
Contributor

Is it necessary for this function to be public?

This comment has been minimized.

@yidawang

yidawang Jan 13, 2017
Author Member

May or may not be used by others in the future. But for now, let me make it private

@mihaic
mihaic approved these changes Jan 13, 2017
@mihaic mihaic merged commit 6d4c11d into brainiak:master Jan 13, 2017
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
linux Build finished.
Details
macos Build finished.
Details
@yidawang yidawang deleted the yidawang:corr_comp branch Jan 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants