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

added correlation function to statistics #3015

Merged
merged 9 commits into from
Sep 1, 2023

Conversation

vrushaket
Copy link
Contributor

@vrushaket vrushaket commented Aug 23, 2023

This PR adds correlation function to statistics and solves the issue "Correlation function #2624"

Copy link
Owner

@josdejong josdejong left a comment

Choose a reason for hiding this comment

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

Thanks for your PR! I made a few inline comments, can you have a look at those?

src/function/statistics/correlation.js Outdated Show resolved Hide resolved
test/unit-tests/function/statistics/correlation.test.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@vrushaket vrushaket force-pushed the correlation-function branch from 1d1d8cb to d6a403a Compare August 23, 2023 17:46
@vrushaket vrushaket requested a review from josdejong August 23, 2023 18:00
Copy link
Owner

@josdejong josdejong left a comment

Choose a reason for hiding this comment

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

Thanks @vrushaket , the function looks complete now and well tested.

One last thing: can you please make sure the function names and file names all consistently use "corr" and not "correlation"? I.e. rename correlation.js to corr.js, rename createCorrelation to createCorr, etc.

@vrushaket vrushaket force-pushed the correlation-function branch from 1e7afe6 to 37359fb Compare September 1, 2023 03:50
@vrushaket vrushaket requested a review from josdejong September 1, 2023 04:00
@vrushaket
Copy link
Contributor Author

Thanks @vrushaket , the function looks complete now and well tested.

One last thing: can you please make sure the function names and file names all consistently use "corr" and not "correlation"? I.e. rename correlation.js to corr.js, rename createCorrelation to createCorr, etc.

@josdejong as per asked changes, I have renamed correlation.js to corr.js, and createCorrelation to createCorr. Please check.

@josdejong
Copy link
Owner

Thanks, looks good.

I see one unit test is failing, I think it will be fixed by replacing

math.corr(matrix([[1, 2.2, 3, 4.8, 5], [1, 2, 3, 4, 5]]), matrix([[4, 5.3, 6.6, 7, 8], [1, 2, 3, 4, 5]])) // returns [0.9569941688503644, 1])

with

math.corr(math.matrix([[1, 2.2, 3, 4.8, 5], [1, 2, 3, 4, 5]]), math.matrix([[4, 5.3, 6.6, 7, 8], [1, 2, 3, 4, 5]])) // returns [0.9569941688503644, 1])

Can you try that?

@vrushaket
Copy link
Contributor Author

Thanks, looks good.

I see one unit test is failing, I think it will be fixed by replacing

math.corr(matrix([[1, 2.2, 3, 4.8, 5], [1, 2, 3, 4, 5]]), matrix([[4, 5.3, 6.6, 7, 8], [1, 2, 3, 4, 5]])) // returns [0.9569941688503644, 1])

with

math.corr(math.matrix([[1, 2.2, 3, 4.8, 5], [1, 2, 3, 4, 5]]), math.matrix([[4, 5.3, 6.6, 7, 8], [1, 2, 3, 4, 5]])) // returns [0.9569941688503644, 1])

Can you try that?

@josdejong I have made the necessary changes to pass the test case to ensure correct output of the function. Please check.

@josdejong
Copy link
Owner

Thanks, all tests pass now 🎉

@josdejong josdejong merged commit 1ee8733 into josdejong:develop Sep 1, 2023
7 checks passed
@josdejong
Copy link
Owner

Published now in [email protected]. Thanks again!

@josdejong josdejong mentioned this pull request Sep 5, 2023
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