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

Use numpy divide in explained variance #1691

Merged
merged 3 commits into from
Mar 31, 2022
Merged

Conversation

anargyri
Copy link
Collaborator

@anargyri anargyri commented Mar 31, 2022

Description

Useful in cases when all true labels are the same and hence explained variance should equal minus infinity.

Related Issues

Checklist:

  • I have followed the contribution guidelines and code style for this project.
  • I have added tests covering my contributions.
  • I have updated the documentation accordingly.
  • This PR is being made to staging branch and not to main branch.

@@ -158,7 +159,8 @@ def exp_var(self):
0
]
var2 = self.y_pred_true.selectExpr("variance(label)").collect()[0][0]
return 1 - var1 / var2
# numpy divide is more tolerant to var2 being zero
return 1 - np.divide(var1, var2)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm looking at the test here: https://github.com/microsoft/recommenders/blob/main/tests/unit/recommenders/evaluation/test_spark_evaluation.py#L239, should we maybe add the edge case for when var2 is zero?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe, although it is a very unusual case in practice (everything in the data is rated equally).

@miguelgfierro miguelgfierro merged commit f52cef2 into staging Mar 31, 2022
@miguelgfierro miguelgfierro deleted the andreas/exp_var branch March 31, 2022 15:15
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