Weighted Pearson, Spearman, and R2 score #1759

matsumotosan · 2023-05-06T12:43:04Z

What does this PR do?

Before submitting

Was this discussed/agreed via a Github issue? (no need for typos and docs improvements)
Did you read the contributor guideline, Pull Request section?
Did you make sure to update the docs?
Did you write any new necessary tests?

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 🙃

matsumotosan · 2023-05-06T13:11:53Z

I’m adding a weighted version of Pearson right now and updated the pearson_corrcoef function to take in an additional optional argument weights. When testing, I am getting a failure due to pearson_corrcoef() got an unexpected keyword argument 'weights' even though I’ve modified the function definition for pearson_corrcoef.

I tried debugging test_pearson_corrcoef_functional to check the signature of the imported pearson_corrcoef and got <Signature (preds: torch.Tensor, target: torch.Tensor) -> torch.Tensor> (missing weights: torch.Tensor).

SkafteNicki · 2023-05-06T13:52:19Z

src/torchmetrics/functional/regression/utils.py

+    if weights.ndim == preds.ndim:
+        raise ValueError(
+            f"Expected `n_samples` of preds to match the length of `weights`,"
+            f" but got {preds.shape[0]} and {len(weights)}."
+        )
+    else:
+        raise ValueError("")


Suggested change

if weights.ndim == preds.ndim:

raise ValueError(

f"Expected `n_samples` of preds to match the length of `weights`,"

f" but got {preds.shape[0]} and {len(weights)}."

)

else:

raise ValueError("")

if weights.shape != preds.shape:

raise ValueError(

f"Expected `n_samples` of preds to match the length of `weights`,"

f" but got {preds.shape[0]} and {len(weights)}."

)

I think this check needs to be even more restrictive such that they can be multiplied together or else I am missing something.
Also I do not think there should be an else statement.

I split the check into single and multi-output cases:

if single output (preds.ndim=1), preds and weights must have same shape

if multi-output (preds.ndim=2)

if weights.ndim=2, preds and weights must have same shape

if weight.ndim=1, preds.shape[0] has to be equal to len(weights)

Okay, that makes sense to me, but the implementation then needs to reflect that. Currently, if weight.ndim=1 and preds.ndim=2 the code will just fail with an value error.

src/torchmetrics/regression/pearson.py

SkafteNicki · 2023-05-06T13:55:07Z

tests/unittests/regression/test_pearson.py

 def _scipy_pearson(preds, target):
    if preds.ndim == 2:
        return [pearsonr(t.numpy(), p.numpy())[0] for t, p in zip(target.T, preds.T)]
    return pearsonr(target.numpy(), preds.numpy())[0]


+def _weighted_pearson(preds, target, weights):
+    if preds.ndim == 2:
+        return [pearsonr(t.numpy(), p.numpy())[0] for t, p in zip(target.T, preds.T)]


should this not depend on weights?

tests/unittests/regression/test_pearson.py

SkafteNicki · 2023-05-10T06:33:00Z

@matsumotosan I have taken care of the testing now, such that inputs gets correctly send to the respective input.
Now the tests are only failing due to some values error in relation to the shape checking.

Borda · 2023-08-08T07:06:17Z

@matsumotosan @SkafteNicki, how is it going here? Could we have it for the next 1.1 release?

matsumotosan · 2023-08-18T06:48:15Z

The docstring for final_aggregation contains an incomplete reference to some sort of equation to compute the aggregated statistics. Would anyone happen to know where this is?

for more information, see https://pre-commit.ci

* fix * changelog

for more information, see https://pre-commit.ci

* fix * changelog

for more information, see https://pre-commit.ci

Borda · 2023-09-19T13:40:39Z

@matsumotosan run rebase from master so pls check if I resolved all conflict property...

Borda · 2023-09-19T13:41:40Z

src/torchmetrics/functional/regression/pearson.py

    if cond:
-        mx_new = (n_prior * mean_x + preds.sum(0)) / (n_prior + n_obs)
-        my_new = (n_prior * mean_y + target.sum(0)) / (n_prior + n_obs)
+        if weights is None:
+            mx_new = (n_prior * mean_x + preds.sum(0)) / (n_prior + n_obs)
+            my_new = (n_prior * mean_y + target.sum(0)) / (n_prior + n_obs)
+        else:
+            mx_new = (n_prior * mean_x + torch.matmul(weights, preds)) / (n_prior + n_obs)
+            my_new = (n_prior * mean_y + torch.matmul(weights, target)) / (n_prior + n_obs)
    else:
-        mx_new = preds.mean(0)
-        my_new = target.mean(0)
+        if weights is None:
+            mx_new = preds.mean(0)
+            my_new = target.mean(0)
+        else:
+            mx_new = torch.matmul(weights, preds) / weights.sum()
+            my_new = torch.matmul(weights, target) / weights.sum()


Can we rather have have if weights is NOne to make it full of ones to have no effect but the code simpler

SkafteNicki reviewed May 6, 2023

View reviewed changes

SkafteNicki assigned matsumotosan May 6, 2023

SkafteNicki added the enhancement New feature or request label May 6, 2023

SkafteNicki added this to the v1.0.0 milestone May 6, 2023

matsumotosan requested a review from SkafteNicki May 8, 2023 01:38

SkafteNicki modified the milestones: v1.0.0, future Jun 2, 2023

Borda force-pushed the master branch from 696e673 to f6760b5 Compare August 22, 2023 20:19

Borda modified the milestones: future, v1.2.0 Aug 24, 2023

SkafteNicki modified the milestones: v1.2.0, v1.3.0 Sep 15, 2023

github-actions bot removed the topic: MModal label Sep 19, 2023

matsumotosan and others added 19 commits September 19, 2023 15:26

pearson_corrcoeff signature bug

1b10bb1

[pre-commit.ci] auto fixes from pre-commit.com hooks

7893989

for more information, see https://pre-commit.ci

fix test interface

562de40

check if preds and weights have compatible dims

1ff8997

Remove unnecessary else statement

bda951d

[pre-commit.ci] auto fixes from pre-commit.com hooks

877fedb

for more information, see https://pre-commit.ci

division op not needed

2c5938d

fix imports

f8d03d3

functional tests passing

bbba0bb

[pre-commit.ci] auto fixes from pre-commit.com hooks

101ae93

for more information, see https://pre-commit.ci

[pre-commit.ci] auto fixes from pre-commit.com hooks

a4ccfe5

for more information, see https://pre-commit.ci

Fix single update in pearson corrcoef (Lightning-AI#2019)

02cb6f0

* fix * changelog

pearson_corrcoeff signature bug

b265940

fix test interface

16e3779

fix imports

3c57e87

functional tests passing

5352988

[pre-commit.ci] auto fixes from pre-commit.com hooks

cd183a8

for more information, see https://pre-commit.ci

[pre-commit.ci] auto fixes from pre-commit.com hooks

48fc353

for more information, see https://pre-commit.ci

Fix single update in pearson corrcoef (Lightning-AI#2019)

e2e800f

* fix * changelog

Borda force-pushed the 1235-weighted-reg-coeffs branch from 6a8603b to e2e800f Compare September 19, 2023 13:39

[pre-commit.ci] auto fixes from pre-commit.com hooks

ae73a93

for more information, see https://pre-commit.ci

Borda reviewed Sep 19, 2023

View reviewed changes

Borda force-pushed the master branch from 04684b9 to 80a7b68 Compare January 11, 2024 12:55

Borda modified the milestones: v1.3.0, v1.4.0 Jan 11, 2024

Borda force-pushed the master branch from 306bb3d to 4ed43e6 Compare March 14, 2024 12:36

Borda force-pushed the master branch 2 times, most recently from d0a5568 to 9fc79ae Compare September 16, 2024 08:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Weighted Pearson, Spearman, and R2 score #1759

Weighted Pearson, Spearman, and R2 score #1759

matsumotosan commented May 6, 2023

matsumotosan commented May 6, 2023

SkafteNicki May 6, 2023

matsumotosan May 7, 2023

SkafteNicki May 10, 2023

SkafteNicki May 6, 2023

SkafteNicki commented May 10, 2023

Borda commented Aug 8, 2023

matsumotosan commented Aug 18, 2023

Borda commented Sep 19, 2023

Borda Sep 19, 2023

Weighted Pearson, Spearman, and R2 score #1759

Are you sure you want to change the base?

Weighted Pearson, Spearman, and R2 score #1759

Conversation

matsumotosan commented May 6, 2023

What does this PR do?

Did you have fun?

matsumotosan commented May 6, 2023

SkafteNicki May 6, 2023

Choose a reason for hiding this comment

matsumotosan May 7, 2023

Choose a reason for hiding this comment

SkafteNicki May 10, 2023

Choose a reason for hiding this comment

SkafteNicki May 6, 2023

Choose a reason for hiding this comment

SkafteNicki commented May 10, 2023

Borda commented Aug 8, 2023

matsumotosan commented Aug 18, 2023

Borda commented Sep 19, 2023

Borda Sep 19, 2023

Choose a reason for hiding this comment