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.
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
Rate games accounting for first move advantage (fix #6818) #16281
Rate games accounting for first move advantage (fix #6818) #16281
Changes from 33 commits
d9d4b2a
aa180ce
6194f07
1f2aff7
31ce96c
d22d638
8b5182f
759d6d4
e3ee697
3f6bbb6
09692bf
697a23b
421560c
da2b525
ef02272
d31bc4a
300d191
2742c8c
887fea5
3e254fd
8e89323
657c1e6
aaf28c4
e4195d4
49a6d9e
69b6e34
41a260b
9e31c15
9ea827f
3e92964
f6e8bb4
850f2f8
2774b03
685e069
2f0db66
fad133d
6270982
3083ac3
82a7c74
31543bd
32e91cf
3f71c58
85ecdac
f9e34be
d10fc58
0b4948e
1bc6e04
aa374d3
22a850a
e366158
485d459
9aecdde
8b752af
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
this is strange, or I'm misunderstanding something.
This
updateRatings
function now takes an additional argumentplayers
.That info used to come from
results
which also contains the players asresults.players
.Which means there are now 2 sources of truth for the players. Both
players
andresults.players
.I see that
results.players
has been made private and is completely unused (?!) but it is still a code smell in my opinion, and a potential source of bugs.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.
Indeed, this PR exposes what in my mind was previously a code smell:
RatingCalculator.scala
looking atresults.players
to figure out "which ratings need to be updated?" was unnecessarily complex (besides that previous code being confusing in other ways with trying to figure out if a player played any games in thatRatingPeriodResults
or not).Tutor could have exercises with fixed ratings: in fact, in chess literature (books, magazines, CDs, other training materials) exercises are provided with "if you get this score, we estimate your rating is X." But this PR might be prematurely forcing us to make that design decision.
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.
For others' benefit; we discussed on stream that we don't think
results.players
needs to exist, but it's an artifact from a previous reference implementation prematurely optimized straight from java hell. https://xkcd.com/2347/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.
Regarding
result.players
, classResult
is perhaps the world's least efficient implementation mappingPair[Rating, Pair[OpponentRating, Score]]
or maybeMap[Rating, Pair[OpponentRating, Score]]
(for a pair of players playing a game, for a player and a puzzle, or for a player attempting a fixed-rating tutor exercise). At present my head hurts trying to think about it.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.
Oh wait...
Result
should actually beTuple[OpponentRating, Score]
and there's no need for any functions. Sure, a game has a winner and a loser, so a game should produce aPair[Result]
.