add rs.utils.weighted_pearsonr and tests
#189
Conversation
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
reciprocalspaceship/utils/stats.py
Outdated
|
|
||
| Note | ||
| ---- | ||
| x, y, and w may be arbitrarily batched. the correlation coefficient will be computed pairwise along the last axis. |
There was a problem hiding this comment.
I don't quite understand the use of "batched" in this context. Perhaps an example with the corresponding array shapes would help?
|
|
||
| import reciprocalspaceship as rs | ||
|
|
||
|
|
There was a problem hiding this comment.
Your tests look good, but are there any simple cases that can be used to test the output for non uniform weight cases?
Right now the tests only compare to scipy (requires np.ones weights) or tests execution. Is there a good test for correctness in the more general case?
There was a problem hiding this comment.
i don't know any. i'm happy to incorporate any examples you can find.
There was a problem hiding this comment.
yep -- unsure. I'm ok with this as is given there isn't another reference implementation for the general case.
There was a problem hiding this comment.
i can add a regression test. is that a good idea?
There was a problem hiding this comment.
Perhaps -- do you mean by including a test against the current implementation? Also, is useful to test against scipy using uniform weights that aren't np.ones? Any uniform array should be equivalent, right?
There was a problem hiding this comment.
yes test against the current implementation.
There was a problem hiding this comment.
okay i added a test against non-one weights and a test against a current result. i'll merge after ci.
There was a problem hiding this comment.
I think this looks good -- two small requests:
- I was a bit confused by your use of "batched" in the docstring. Please clarify with an example or by saying something about the shapes of the input and the expected output.
- I am wondering if there's an additional test of correctness that can be done for the non-uniform weights case. Right now, that only gets tested for execution, but not correctness.
for more information, see https://pre-commit.ci
Codecov ReportBase: 98.35% // Head: 98.36% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #189 +/- ##
==========================================
+ Coverage 98.35% 98.36% +0.01%
==========================================
Files 44 44
Lines 1764 1775 +11
==========================================
+ Hits 1735 1746 +11
Misses 29 29
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
…procalspaceship into weighted_pearsonr
for more information, see https://pre-commit.ci
I wrote a function to compute weighted Pearson correlation coefficients and added it to
utils/stats.py.