-
Notifications
You must be signed in to change notification settings - Fork 411
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
lightweight explained variance #68
Conversation
@thomasgaudelet could you please explain the intention behind this PR? |
Sorry thought this was sufficient The issue with the current version of explained variance is that it stores all predictions and ground truths until the compute step (as shown by the RankZero warning). This is fine for small datasets but it doesn't scale. The PR replaces storing potentially large vectors with just tracking 5 sums. It's in a similar spirit to the current implementation of MSE and the likes. |
@thomasgaudelet, thanks for the extra explaination, great improvement :] |
Codecov Report
@@ Coverage Diff @@
## master #68 +/- ##
===========================================
- Coverage 97.07% 78.62% -18.45%
===========================================
Files 118 59 -59
Lines 3898 1951 -1947
===========================================
- Hits 3784 1534 -2250
- Misses 114 417 +303
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
@SkafteNicki done, there was a couple things mishandled but seems to pass all now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you also add entry under the Changed
section in changelog?
Something like:
"Changed ExplainedVariance
from storing all preds/targets to tracking 5 statistics..."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Before submitting
What does this PR do?
Propose a lightweight version of explained variance without storing all predictions and ground truth up to compute.
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 🙃