-
Notifications
You must be signed in to change notification settings - Fork 6
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
Incorporate komi into ratings (particularly for small boards) #46
Closed
dexonsmith
wants to merge
10
commits into
online-go:master
from
dexonsmith:incorporate-komi-into-ratings
Closed
Changes from 9 commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
f1c7a13
Include all boards in compact stats
dexonsmith ab475ab
Add komi integration to ratings math
dexonsmith a6d8ba5
Export get_handicap_rank_difference from RatingMath
dexonsmith 55dad80
Use get_handicap_rank_difference when ignoring games
dexonsmith 2197d9e
WIP: Use get_handicap_rank_difference for skipping games
dexonsmith 9895341
Merge branch 'infer-komi-for-aga-and-egf' into incorporate-komi-into-…
dexonsmith 0d239bc
Merge branch 'infer-komi-for-aga-and-egf' into incorporate-komi-into-…
dexonsmith 65d3dc0
Merge remote-tracking branch 'origin/centralize-skipping-logic' into …
dexonsmith ce5796d
Merge remote-tracking branch 'upstream/master' into incorporate-komi-…
dexonsmith 923ba08
Fix an off-by-one bug in get_handicap_rank_difference
dexonsmith File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I'm trying to ponder here, I think the reason why I didn't clamp the rank between 30k-9d for this was intentional (but not necessarily correct, that decision is certainly open to discussion).
My gut says that by clamping this we're going to introduce some unnecessary noise. There are legitimately terrible players (including purposefully terrible beginner bots), and there are some legitimately >9d "players" (think every strong KataGo bot). I call out the bots in specific because they have a whole lot of games and can actually move the needle when it comes to where a lot of players land with their ratings. While we clamp those for display to the users, that's more out of convention and UX than a good mathematical reason. I think for the purposes of doing good ratings I think it does a bit of harm to pretend like everyone is squished within those ranks.
Again, open to discussion if you have a good reason to clamp them.
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.
Main reasoning here was practical, to avoid a MathDomainError (can't remember if it was in
rank_to_rating
or in the subsequent ratings update calculation). I did NOT play around with the bounds of the clamping... maybe -10 (40k) to 45 would be sufficient to avoid the MathDomainError. Maybe these clamp values could be parameterized? Maybe we should depend on SkipLogic to preclude the need for this?In terms of handling players with true ranks outside the UX, I'm not sure how reasonable results can be if the range (output) of
rating_to_rank
and the domain (input) ofrank_to_rating
don't match. But that's the kind of data I think I was seeing (after applying small board handicap).Note that I saw the math coming up with effective handicap as high as 52, from an 8-stone 9x9 game early in OGS history (not sure there was only one....). (Maybe that error predated the
SkipLogic
change and wouldn't be necessary now...)BTW, IMO,:
rank_to_rating
/rating_to_rank
calculations, we should choose a domain and range forranks
that match. Either we clamp for things outside of that, or don't rate games that handicaps put outside of that, or something.rating_to_rank
might have a player down as 0 or -5, but the UX would show 25k (not 30k or 35k).Not sure how that matches what we have right now...
(Feels a bit like it would be worthwhile to consider more of these changes in isolation... maybe I should split up this PR more after all... not today though)
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.
Don't have much time today, but my initial attempts to reproduce this (by commenting out the mitigation and the new SkipLogic code) haven't been successful...
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.
Converted back to a draft for now— since I can't reproduce the MathDomainError anymore it seems better to remove the clamping for now, and if we add it back it should be parameterized.
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.
Aha, found it, when I temporarily reverted the clamping, I inserted a bug in
get_handicap_adjustment
, which is why I couldn't reproduce.The domain error is on tallygameanalytics:
Anyway, I still think the options can land without fixing the MathDomainError, and then we can decide whether to fix with clamping or to update the TalylGameAnalytics to handle an
expected_win_rate
of 0 (which I assume is what's causing the domain error). Sending a new PR soon.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.
Makes sense, though honestly if we're hitting the domain error it's because our rating code is busted somewhere, so having it blow up isn't necessarily a bad thing (although having a more appropriate error would probably be better in an ideal world).