atomflunder / skillratings

Rust library for popular skill rating algorithms like Elo, Glicko-2, TrueSkill and many more.
https://docs.rs/skillratings/
Apache License 2.0
41 stars 4 forks source link

`glicko_rating_period` is probably supposed to imply `decay_deviation` #3

Closed asyncth closed 1 year ago

asyncth commented 1 year ago

As far as I understand it, Glicko paper requires to increase RD at the beginning of every rating period in Step 1 before changing it again in Step 2, regardless of whether the player played games in previous rating period or not, since it is said that Step 1 should be done for each player and Step 2 specifies RD as a rating deviation from Step 1:

Step 1. Determine the rating and RD for each player at the start of the new rating period based on their rating and RD at the end of the previous period.

[In Step 2] Assume that the player’s pre-period rating is r, and the ratings deviation is RD determined from Step 1.

Which I believe means that glicko_rating_period needs to run step 1 as well, the same equation that decay_deviation uses. This would be similar to Glicko-2 where Step 6 is ran in both glicko2 and glicko2_rating_period.

atomflunder commented 1 year ago

Hi, thanks for the bug report!

I think you are right, this seems to be an oversight. If you want to submit a PR to fix this yourself, you are welcome to. Otherwise I will do so sometime later this weekend.

I think it would make sense to run the same calculation in the normal glicko function, too. Since it kind of is a rating period with one match, in a sense.

asyncth commented 1 year ago

Trying to fix this now. I wanted to put step 1 calculation into new_deviation functions, but calculating the new rating in step 2 seems to require deviation that was calculated in step 1, but not yet changed by step 2, which means that I probably can't just put the calculation into new_deviation and re-order calculations of new deviation and new rating in glicko function. Should I simply put it into glicko and perhaps change the name of the deviation argument of new_deviation and new_rating functions to something like pre_deviation?

atomflunder commented 1 year ago

Should I simply put it into glicko and perhaps change the name of the deviation argument ofnew_deviation and new_rating functions to something like pre_deviation?

Yes, I think that would make the most sense. The calculated RD from step 1 also would be needed in the g_value function if I understand it correctly. Thanks for fixing it!

asyncth commented 1 year ago

Also, I assume this will break the test_glicko_rating_period test, the paper says it's meant to demonstrate step 2 and it doesn't follow step 1. What do I do with it?

atomflunder commented 1 year ago

Also, I assume this will break the test_glicko_rating_period test, the paper says it's meant to demonstrate step 2 and it doesn't follow step 1. What do I do with it?

We could either pass in a value that will equal the same RD after calculation (seems to be `~190) and round the outcome, or just change the outcome of the test. I think I would prefer solution 1 but either is fine.

Edit: So I with solution 1 I meant setting the RD like this should yield the same results, maybe with a slight rounding error:

let player = GlickoRating {
    rating: 1500.0,
    deviation: (200.0_f64.powi(2) - 62.3_f64.powi(2)).sqrt(),
};
asyncth commented 1 year ago

I'll do the first solution for test_glicko_rating_period, what about test_glicko?

atomflunder commented 1 year ago

I'll do the first solution for test_glicko_rating_period, what about test_glicko?

I think just changing the outcome is the best option there, since it is not based off of any official example.