Skip to content

Conversation

matsumotosan
Copy link
Contributor

@matsumotosan matsumotosan commented Jun 16, 2025

What does this PR do?

Fixes part of #1235

New PR for #1759

  • Create separate class for weighted Pearson (similar to weighted MAPE)

previous PR tried to create single class for weighted and unweighted Pearson but that could result in someone trying to use the metric initialized as unweighted and later change to weighted (and vice versa)

  • 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 🙃

@Borda
Copy link
Contributor

Borda commented Jun 18, 2025

@matsumotosan how is it going here? 🦩

@matsumotosan
Copy link
Contributor Author

@Borda Going to move over the test cases from the previous PR. Now that the weighted and unweighted metrics are separate, tests should be easier to write.

@Borda
Copy link
Contributor

Borda commented Jun 23, 2025

Going to move over the test cases from the previous PR. Now that the weighted and unweighted metrics are separate, tests should be easier to write.

sounds cool :)

@Borda
Copy link
Contributor

Borda commented Jun 30, 2025

it would be great to ave this landed and then we can do another feature release :)

@matsumotosan matsumotosan requested a review from lantiga as a code owner June 30, 2025 20:08
@github-actions github-actions bot added the documentation Improvements or additions to documentation label Jun 30, 2025
@matsumotosan matsumotosan self-assigned this Jun 30, 2025
@Borda
Copy link
Contributor

Borda commented Jul 8, 2025

@matsumotosan do you have a time estimate for finishing this one?
There is not pressure, I just need to understand if we should wait for it for the next incoming release or just leave it for the next one 🦩

@matsumotosan
Copy link
Contributor Author

@Borda Working on it right now, I think I might need a week.

@Borda
Copy link
Contributor

Borda commented Jul 18, 2025

Working on it right now, I think I might need a week.

@matsumotosan please check Nicki comments :)

elif check_batch and not metric.dist_sync_on_step:
batch_kwargs_update = {
k: v.cpu() if isinstance(v, Tensor) else v
k: v[i].cpu() if isinstance(v, Tensor) else v
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change is breaking class tests for metrics like precision, recall, MAP, MRR, fallout.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Borda Borda requested a review from Copilot September 19, 2025 19:50
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation topic: Regress

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants