-
Notifications
You must be signed in to change notification settings - Fork 2
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
[WIP] Implements ELO rating for sunspotter #26
Conversation
Hello @Raahul-Singh! Thanks for updating this PR.
Comment last updated at 2020-06-30 10:51:35 UTC |
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.
Very good @Raahul-Singh I'm very happy with the quality shown in here! Well done. I've made some small comments (many are more of a conversation with myself 😅), you don't need to agree with them all.
Finally, maybe we want to keep this more general, then we may break this into ELO only and ELO for sunspotter.
Rankings initially:
The matches are
Rankings after each player have had one match:
|
@dpshelio if this looks good, I'll run it on the 14 years dataset and leave it to run overnight. |
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.
Nice tidy code @Raahul-Singh! Only a couple comments from me as I think @dpshelio covered most of it 👍
fe80767
to
eb4bd83
Compare
d0e8f9d
to
70f61fe
Compare
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.
It seems good to me, so you can go ahead and merge it. You can create issues for the test data or names of variables to solve them later.
🌞 drying the update of state dictionaries
Co-authored-by: David Pérez-Suárez <[email protected]>
This is the python implementation of the ELO score.
I need to figure out how to test this. Running it on the entire dataset will take a long time, and we cannot verify it otherwise.
Probably need to create some dummy data.
Finally, this may have some minute bugs that may have escaped manual testing. These will be sorted after we figure out testing.
Fixes: #7