Rate games accounting for first move advantage (fix #6818) #16281

ddugovic · 2024-10-27T21:39:02Z

Case studies measure a first move advantage of 35 Elo, for example:
2002 - https://en.chessbase.com/post/the-sonas-rating-formula-better-than-elo
2024 - https://www.robweir.com/blog/2014/01/first-move-advantage-in-chess.html

This PR does not tune tau (see http://www.glicko.net/glicko/glicko2.pdf ),
although doing so may help stabilize variant and ultrabullet ratings.

niklasf · 2024-10-29T19:34:48Z

Love to see this!

I analysed this a while back and concluded that adding this parameter is a significant improvement, but somehow never got around to implementing it.

It's important to get the parameter right. If 35 overshoots too much, it could lead to less accurate results than 0.

It should be possible to calculate the optimum value from aggregate results, like opening explorer data. We want that White's average score equals the rating system prediction for a game between two stable players at 1500.

ddugovic · 2024-10-30T00:55:03Z

Sampling rated blitz games with rating=1500, I see:

20,707 (49.7%) white wins
19,802 (47.5%) black wins
1,117 (2.7%) draws

These numbers don't match the Opening Explorer numbers, or any other case study I could find. I selected blitz games because the rapid rating distribution is skewed, and the blitz rating distribution is not skewed.

Using this calculator, I see that an Elo difference of +7.786 produces an expected score of 0.510870.

ornicar · 2024-10-30T11:36:55Z

modules/rating/src/main/glicko2/Result.scala


-final class GameResult(winner: Rating, loser: Rating, isDraw: Boolean) extends Result:
+final class GameResult(val first: Rating, val second: Rating, outcome: Option[Boolean]) extends Result:


I'm concerned that you changed the semantics of this class while keeping typing compatibility.

winner: Rating, loser: Rating and val first: Rating, val second: Rating have the same types, but mean different things. Code using it might break without the compiler noticing.

I reckon the class should at least have a different name, to force us to review and fix the code wherever it's used.

Good point. I changed the only other consumer of this class (PuzzleFinisher.scala) to consume BinaryResult instead.

I tried to think of a better name than GameResult to describe this concept... I needed a way to identify which player had the first move advantage.

modules/puzzle/src/main/PuzzleFinisher.scala

ornicar · 2024-10-30T11:49:22Z

modules/puzzle/src/main/PuzzleFinisher.scala

-  private val VOLATILITY = lila.rating.Glicko.default.volatility
-  private val TAU        = 0.75d
-  private val calculator = glicko2.RatingCalculator(VOLATILITY, TAU)
+  private val calculator = lila.rating.Glicko.system


what the fudge... there was a horrible bug here.

private val calculator = glicko2.RatingCalculator(VOLATILITY, TAU)

was completely wrong here, as the constructor goes RatingCalculator(tau: Double = 0.75d, ratingPeriodsPerDay: Double = 0).

And that's what happens when we use weak types like Double. Arguments get swapped during some code change and no-one notices.

Gonna change these to proper opaque type so that it doesn't happen again.

Goodness, that is quite a mess and I feel somewhat bad for having introduced it years ago.

Parameter tau being near-zero may have caused solvers' volatility to increase slower when on a hot streak:

Smaller values of τ prevent the volatility measures from changing by large amounts, which in turn prevent enormous changes in ratings based on very improbable results.
http://www.glicko.net/glicko/glicko2.pdf

Parameter ratingPeriodsPerDay being 0;75d instead of 0.21436d may have caused solvers' RD to increase faster.

Note that I wasn't blaming you for the bug. In fact I didn't check who introduced it and assumed it was me.
Thanks for fixing it tho.

ornicar · 2024-10-30T11:52:26Z

looks like I cannot push to this PR, that's a bit unpractical

ddugovic · 2024-10-30T17:33:09Z

I'm trying to grant you collaborator access as well as trying to grant you access here

niklasf · 2024-10-30T17:38:20Z

modules/rating/src/main/glicko2/Result.scala


-  def getOpponent(player: Rating): Rating
+  def getAdvantage(advantage: Double, player: Rating): Double =
+    if player == first then advantage / 2.0d else -advantage / 2.0d


Aside: It's a bit insane that players are identified by their exact Rating. Not introduced by this PR, though.

Indeed, there is a dilemma when two players have the exact same Rating. (I can't tell at the moment whether this PR either creates or fixes that edge case.)

yeah that comes from https://github.com/goochjs/glicko2. We could fix it.

I'll try to fix it... but also, there's a case of dependency inversion since RatingCalculator shouldn't need to inspect RatingPeriodResults to identify what players, puzzles, tutor-exercises, etc. need ratings updated.

Trait RatingPeriodResults should be replaceable by List[Result], because each consumer of RatingCalcuator#updateRatings already has that context, especially because we're only rating games and puzzles serially (one at a time) anyway (except for modules/tutor/src/test/GlickoTest.scala which rates 2 results concurrently).

in any case this PR is not the time and place to fix it.

ornicar · 2024-10-30T19:51:56Z

[EDIT] I'm able to push after accepting the github invitation to contribute.

~~IDK why I still can't push to this PR~~

git remote -v
origin	https://github.com/lichess-org/lila.git (fetch)
origin	https://github.com/lichess-org/lila.git (push)
lishogi	https://github.com/ddugovic/lishogi.git (fetch)
lishogi	https://github.com/ddugovic/lishogi.git (push)

git push --set-upstream lishogi glicko-first-move
remote: Permission to ddugovic/lishogi.git denied to ornicar.
fatal: unable to access 'https://github.com/ddugovic/lishogi.git/': The requested URL returned error: 403

It happened before on a couple PRs and has remained a mystery.

ornicar · 2024-10-30T19:54:07Z

sbt scalafmtAll fixes CI

ddugovic · 2024-11-02T00:04:35Z

Given excitement in our team channel about crazyhouse, I did a little analysis (and maybe should update this PR):

This month, 1228 rated crazyhouse games were played, with 628 (51%) White wins and 575 (47%) Black wins, a 15.171 Elo advantage (as compared to 7.786 Elo for standard blitz).

fixes clicking the reset password email link after obtaining a gdpr erasure

ornicar · 2024-11-17T14:11:16Z

modules/rating/src/main/glicko2/RatingCalculator.scala

+      players: Iterable[Rating],
+      results: RatingPeriodResults[?],
+      skipDeviationIncrease: Boolean = false
+  ) =


this is strange, or I'm misunderstanding something.

This updateRatings function now takes an additional argument players.

That info used to come from results which also contains the players as results.players.

Which means there are now 2 sources of truth for the players. Both players and results.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.

Indeed, this PR exposes what in my mind was previously a code smell: RatingCalculator.scala looking at results.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 that RatingPeriodResults 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.

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/

Regarding result.players, class Result is perhaps the world's least efficient implementation mapping Pair[Rating, Pair[OpponentRating, Score]] or maybe Map[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.

Oh wait... Result should actually be Tuple[OpponentRating, Score] and there's no need for any functions. Sure, a game has a winner and a loser, so a game should produce a Pair[Result].

ornicar · 2024-11-17T14:48:29Z

modules/round/src/main/PerfsUpdater.scala

    val results = glicko2.GameRatingPeriodResults(
      List(
        game.winnerColor match
-          case None              => glicko2.GameResult(white, black, true)
-          case Some(chess.White) => glicko2.GameResult(white, black, false)
-          case Some(chess.Black) => glicko2.GameResult(black, white, false)


Here we go from

case Some(chess.White) => glicko2.GameResult(white, black, false) case Some(chess.Black) => glicko2.GameResult(black, white, false)

to

case Some(chess.White) => glicko2.GameResult(white, black, Some(true)) case Some(chess.Black) => glicko2.GameResult(black, white, Some(false))

which I'm pretty sure has a bug, not fixed by more recent commits

I've made a best effort to correct this (and pushed my commits), and am overwhelmed trying to understand compile errors.

This reverts commit 22a850a.

ornicar · 2024-11-19T15:59:17Z

I'm making a pure glicko calculator API, moving it to scalachess, and integrating it back into lila

ddugovic · 2024-11-21T04:43:54Z

I'm making a pure glicko calculator API, moving it to scalachess, and integrating it back into lila

Thanks! I see that PR successfully merged into master!

ddugovic · 2024-11-30T16:42:21Z

On scalachess I'm not a collaborator, so I created lichess-org/scalachess#599

Rate games with first move advantage (fix lichess-org#6818)

d9d4b2a

ddugovic force-pushed the glicko-first-move branch from 8320559 to d9d4b2a Compare October 27, 2024 21:41

ddugovic added 4 commits October 27, 2024 16:57

Code cleanup (reorder parameters)

aa180ce

Allow advantage even for BinaryResult

6194f07

Code cleanup

1f2aff7

Code cleanup

31ce96c

niklasf linked an issue Oct 29, 2024 that may be closed by this pull request

adjust Glicko-2 to account for first move advantage #6818

Open

ddugovic and others added 2 commits October 29, 2024 20:42

Reduce first move advantage to 7.786 Elo

d22d638

Merge branch 'master' into glicko-first-move

8b5182f

ornicar reviewed Oct 30, 2024

View reviewed changes

modules/puzzle/src/main/PuzzleFinisher.scala Outdated Show resolved Hide resolved

ornicar reviewed Oct 30, 2024

View reviewed changes

Merge branch 'master' into glicko-first-move

759d6d4

niklasf reviewed Oct 30, 2024

View reviewed changes

ornicar added 2 commits October 30, 2024 20:42

Merge branch 'master' into glicko-first-move

e3ee697

sbt scalafmtAll

3f6bbb6

ddugovic and others added 7 commits October 31, 2024 04:00

Remove function getParticipants

09692bf

sbt scalafmtAll

697a23b

Merge branch 'master' into glicko-first-move

421560c

scala tweaks while reviewing

da2b525

type safety for glicko2.ColorAdvantage

ef02272

Rename GameResult to DuelResult

d31bc4a

Merge branch 'master' into glicko-first-move

300d191

ornicar added 3 commits November 17, 2024 14:57

better recover from erased users

f6e8bb4

fixes clicking the reset password email link after obtaining a gdpr erasure

rename Glicko.calculator

850f2f8

use chess.Outcome for glicko Result

2774b03

ornicar reviewed Nov 17, 2024

View reviewed changes

ddugovic added 20 commits November 17, 2024 12:49

Delete bug (WIP)

685e069

Delete bug (WIP)

2f0db66

Delete bug (WIP)

fad133d

Delete bug

6270982

Attempt to fix type errors

3083ac3

Attempt to fix type errors

82a7c74

Attempt to fix type errors

31543bd

Attempt to fix type errors

32e91cf

Attempt to fix type errors

3f71c58

Attempt to fix type errors

85ecdac

Attempt to fix type errors

f9e34be

Attempt to fix type errors

d10fc58

scalafmtAll

0b4948e

Attempt to fix type errors

1bc6e04

Strongly type result color

aa374d3

Make RatingPeriodResults immutable

22a850a

Revert "Make RatingPeriodResults immutable"

e366158

This reverts commit 22a850a.

Make Rating immutable

485d459

Rating facade WIP

9aecdde

Rating facade

8b752af

ornicar closed this Nov 20, 2024

ddugovic deleted the glicko-first-move branch November 21, 2024 04:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rate games accounting for first move advantage (fix #6818) #16281

Rate games accounting for first move advantage (fix #6818) #16281

ddugovic commented Oct 27, 2024

niklasf commented Oct 29, 2024

ddugovic commented Oct 30, 2024 •

edited

Loading

ornicar Oct 30, 2024 •

edited

Loading

ddugovic Oct 30, 2024

ornicar Oct 30, 2024

ddugovic Oct 30, 2024

ornicar Oct 30, 2024

ornicar commented Oct 30, 2024

ddugovic commented Oct 30, 2024

niklasf Oct 30, 2024 •

edited

Loading

ddugovic Oct 30, 2024 •

edited

Loading

ornicar Oct 30, 2024

ddugovic Oct 31, 2024

ornicar Oct 31, 2024

ornicar commented Oct 30, 2024 •

edited

Loading

ornicar commented Oct 30, 2024

ddugovic commented Nov 2, 2024 •

edited

Loading

ornicar Nov 17, 2024

ddugovic Nov 17, 2024

ddugovic Nov 17, 2024 •

edited

Loading

ddugovic Nov 17, 2024 •

edited

Loading

ddugovic Nov 17, 2024 •

edited

Loading

ornicar Nov 17, 2024 •

edited

Loading

ddugovic Nov 17, 2024

ornicar commented Nov 19, 2024

ddugovic commented Nov 21, 2024

ddugovic commented Nov 30, 2024


		final class GameResult(winner: Rating, loser: Rating, isDraw: Boolean) extends Result:
		final class GameResult(val first: Rating, val second: Rating, outcome: Option[Boolean]) extends Result:

Rate games accounting for first move advantage (fix #6818) #16281

Rate games accounting for first move advantage (fix #6818) #16281

Conversation

ddugovic commented Oct 27, 2024

niklasf commented Oct 29, 2024

ddugovic commented Oct 30, 2024 • edited Loading

ornicar Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ornicar commented Oct 30, 2024

ddugovic commented Oct 30, 2024

niklasf Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

ddugovic Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ornicar commented Oct 30, 2024 • edited Loading

ornicar commented Oct 30, 2024

ddugovic commented Nov 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ddugovic Nov 17, 2024 • edited Loading

Choose a reason for hiding this comment

ddugovic Nov 17, 2024 • edited Loading

Choose a reason for hiding this comment

ddugovic Nov 17, 2024 • edited Loading

Choose a reason for hiding this comment

ornicar Nov 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ornicar commented Nov 19, 2024

ddugovic commented Nov 21, 2024

ddugovic commented Nov 30, 2024

ddugovic commented Oct 30, 2024 •

edited

Loading

ornicar Oct 30, 2024 •

edited

Loading

niklasf Oct 30, 2024 •

edited

Loading

ddugovic Oct 30, 2024 •

edited

Loading

ornicar commented Oct 30, 2024 •

edited

Loading

ddugovic commented Nov 2, 2024 •

edited

Loading

ddugovic Nov 17, 2024 •

edited

Loading

ddugovic Nov 17, 2024 •

edited

Loading

ddugovic Nov 17, 2024 •

edited

Loading

ornicar Nov 17, 2024 •

edited

Loading