Closed jspspike closed 1 year ago
Hi, thanks for your PR!
This makes a lot of sense to me, one of the main features of this crate is supposed to be switching and comparing between rating systems easily and fast.
I think it looks quite good so far, and I would be happy to merge, but I think if we were to do it, we should probably implement it for every rating system and also every type of algorithm (1v1, Team v Team, etc.). This means to also include RatingSystem
(1v1), RatingPeriodSystem
and MultiTeamRatingSystem
(names are obviously not set in stone) and of course implementing all of those for every algorithm currently available.
I would be more than happy to work on doing that, but I can only start working on it properly this weekend. So please feel free to add to this in whichever way you like in the meantime.
@jspspike Would you be okay with me adding the traits I described above to this existing PR?
While implementing those I did have some thoughts about the method names, for example the rating
method should probably be called something like rate
, in my opinion. Also I think it does make sense to add an uncertainty
to the Rating
trait, as most algorithms included have some form of this in their rating. For edge cases like volatility
with Glicko-2 this would obviously not make sense to include those.
@atomflunder Sure feel free to implement them, you could add directly to this branch. I think your proposals make sense, I guess the concern would be for edge cases where there isn't an uncertainty how to handle those cases. My opinion would be to omit uncertanity
unless all implementations could use it but it's your decision on what you think the traits should be like
I guess the concern would be for edge cases where there isn't an uncertainty how to handle those cases. My opinion would be to omit uncertanity unless all implementations could use it but it's your decision on what you think the traits should be like
My thought process on this was to add a disclaimer in the documentation, and just return 0 for the cases where there is no uncertainty value.
I thought this would be better for the algorithms that rely heavily on the uncertainty value, for example TrueSkill where the original "display rank" is rating - 3* uncertainty
.
To be honest I think both approaches are not 100% optimal, but I personally would prefer adding it. We can always change that later though, if it does end up confusing.
@jspspike I added the traits, let me know what you think and if you spot any mistakes. This should be fairly straightforward to add to the other algorithms from here on out, the hardest part is probably the design choices.
There is also still some documentation and new tests missing, but that won't be a big deal to add.
Edit: Oh and ignore that code coverage report obviously for now.
Patch coverage has no change and project coverage change: -5.73
:warning:
Comparison is base (
398d621
) 100.00% compared to head (73ecde9
) 94.27%.
:exclamation: Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.
@jspspike So if you are okay with it, I would merge this, probably add some more tests and then make a new release for it.
Thank you again for your contribution!
@atomflunder Looks good to me, thanks for taking the time and adding these traits
Each rating system has gone through the trouble having a similar or same interface so I thought it would be beneficial to add traits for the rating system and rating type. This way users could use the crate without having to specify the rating system and easily switch between different ones.
This PR only impls for trueskill at the moment