Skip to content
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

Fix osu! standardised score estimation algorithm violating basic invariants #27513

Merged
merged 3 commits into from Mar 7, 2024

Conversation

bdach
Copy link
Collaborator

@bdach bdach commented Mar 6, 2024

Warning

If this PR is merged, all imported osu! scores will need to be re-verified.

Rationale

As it turns out, the "lower" and "upper" estimates of the combo portion of the score being converted were misnomers. In selected cases (scores with high accuracy but combo being lower than max by more than a few objects) the janky score-based math could overestimate the count of remaining objects in a map. For instance, in the case cited in the issue the numbers worked out something like this:

  • Accuracy: practically 100%, which makes it basically irrelevant in the context of the score-based estimate
  • Max combo on beatmap: 571x
  • Max combo for score: 551x

The score-based estimation attempts to extract a "remaining object count" from score, by doing something along the lines of $\sqrt{571^2 - 551^2}$. That comes out to almost 150. Which leads to the estimation overshooting the total max combo count on the beatmap by some hundred objects.

To curtail this nonsense, enforce some basic invariants:

  • Neither estimate is allowed to exceed maximum achievable
  • Ensure that lower estimate is really lower and upper is really upper by just looking at the values and making sure that is so rather than just saying that it is.

Testing

Spreadsheet: https://docs.google.com/spreadsheets/d/1DPtxdRZINENF2G1ymeZ6uF9EvFKy9KDHJx4rKXmn5FY/edit#gid=855782594

Selected 5 biggest gains by relative %

score estimation on master estimation with this PR actual replay playback comments
https://osu.ppy.sh/scores/osu/4438670234 38,611 74,982 7,196 Gimmick loved map. Change caused by the "lower" estimate being actually higher than the "higher" estimate on master.
https://osu.ppy.sh/scores/osu/2984235726 15,756 24,454 no replay Change caused by the "lower" estimate being actually higher than the "higher" estimate on master.
https://osu.ppy.sh/scores/osu/2232745438 18,188 27,705 no replay Change caused by the "lower" estimate being actually higher than the "higher" estimate on master.
https://osu.ppy.sh/scores/osu/3258433877 56,739 86,189 no replay Change caused by the "lower" estimate being actually higher than the "higher" estimate on master.
https://osu.ppy.sh/scores/osu/4282282416 17,335 25,794 no replay Change caused by the "lower" estimate being actually higher than the "higher" estimate on master.

Selected 5 biggest gains by absolute score

score estimation on master estimation with this PR actual replay playback comments
https://osu.ppy.sh/scores/osu/2295332227 283,504 338,700 no replay Change caused by the "lower" estimate being actually higher than the "higher" estimate on master.
https://osu.ppy.sh/scores/osu/2763122416 347,012 402,169 no replay Change caused by the "lower" estimate being actually higher than the "higher" estimate on master.
https://osu.ppy.sh/scores/osu/2314621963 884,463 934,894 no replay Short map. Change caused by the "lower" estimate being actually higher than the "higher" estimate on master.
https://osu.ppy.sh/scores/osu/4177483953 673,377 722,816 no replay Somewhat short map. Change caused by the "lower" estimate being actually higher than the "higher" estimate on master.
https://osu.ppy.sh/scores/osu/2467617229 239,540 285,508 no replay Change caused by the "lower" estimate being actually higher than the "higher" estimate on master.

Selected 5 biggest losses by relative %

score estimation on master estimation with this PR actual replay playback comments
https://osu.ppy.sh/scores/osu/3225520392 44,033 345 no replay Estimated combo portion on master exceeded 1 by at least an order of magnitude which should be impossible. Was clearly broken.
https://osu.ppy.sh/scores/osu/2078834161 33,930 704 no replay Estimated combo portion on master exceeded 1 by at least an order of magnitude which should be impossible. Was clearly broken.
https://osu.ppy.sh/scores/osu/2965554200 126,744 3,233 no replay Estimated combo portion on master exceeded 1 by at least an order of magnitude which should be impossible. Was clearly broken.
https://osu.ppy.sh/scores/osu/3159965190 31,803 882 no replay Estimated combo portion on master exceeded 1 by at least an order of magnitude which should be impossible. Was clearly broken.
https://osu.ppy.sh/scores/osu/2966247419 136,437 3,967 no replay Estimated combo portion on master exceeded 1 by at least an order of magnitude which should be impossible. Was clearly broken.

Selected 5 biggest losses by absolute score

score estimation on master estimation with this PR actual replay playback comments
https://osu.ppy.sh/scores/osu/4442938195 3,358,923 1,150,845 1,140,191 Estimated total score on master exceeds maximum possible. Estimated combo portion on master exceeded 1 which should be impossible. Was clearly broken.
https://osu.ppy.sh/scores/osu/3002196141 861,713 32,834 no replay The "lower" estimate was actually higher than the "higher" estimate on master. Estimated combo portion on master exceeded 1 by at least an order of magnitude which should be impossible. Was clearly broken.
https://osu.ppy.sh/scores/osu/2401254574 810,024 186,450 no replay The "lower" estimate was actually higher than the "higher" estimate on master. Estimated combo portion on master exceeded 1 which should be impossible. Was clearly broken.
https://osu.ppy.sh/scores/osu/2453690866 429,987 14,585 no replay The "lower" estimate was actually higher than the "higher" estimate on master. Estimated combo portion on master exceeded 1 by at least an order of magnitude which should be impossible. Was clearly broken.
https://osu.ppy.sh/scores/osu/2300168213 328,873 102,388 no replay The "lower" estimate was actually higher than the "higher" estimate on master. Estimated combo portion on master exceeded 1 which should be impossible. Was clearly broken.

cc @Zyfarok

…riants

As it turns out, the "lower" and "upper" estimates of the combo portion
of the score being converted were misnomers. In selected cases
(scores with high accuracy but combo being lower than max by more than
a few objects) the janky score-based math could overestimate the count
of remaining objects in a map. For instance, in one case the numbers
worked out something like this:

- Accuracy: practically 100%
- Max combo on beatmap: 571x
- Max combo for score: 551x

The score-based estimation attempts to extract a "remaining object
count" from score, by doing something along of sqrt(571^2 - 551^2). That
comes out to _almost 150_. Which leads to the estimation overshooting
the total max combo count on the beatmap by some hundred objects.

To curtail this nonsense, enforce some basic invariants:

- Neither estimate is allowed to exceed maximum achievable
- Ensure that lower estimate is really lower and upper is really upper
  by just looking at the values and making sure that is so rather than
  just saying that it is.
@peppy
Copy link
Sponsor Member

peppy commented Mar 7, 2024

I trust that the math here is sound enough based on your word, but could you confirm whether this is going to change every total score number when reverifying (or only the outliers)? That has a large impact on how long this will take to run, and how I approach running it.

@bdach
Copy link
Collaborator Author

bdach commented Mar 7, 2024

whether this is going to change every total score number when reverifying (or only the outliers)?

In theory this should meaningfully touch only the outliers.

@peppy
Copy link
Sponsor Member

peppy commented Mar 7, 2024

@smoogipoo did you want to check this one? else i will get it rolling out with minimal review coverage to keep forward momentum.

Copy link
Contributor

@smoogipoo smoogipoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logic looks fine to me. Haven't gone through the math but this entire thing is over-engineered in my eyes, so...

@smoogipoo smoogipoo merged commit e0fe33a into ppy:master Mar 7, 2024
15 of 17 checks passed
@bdach bdach deleted the osu-score-conversion-bad-very-not-good branch March 7, 2024 08:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

Incorrect legacy score conversion on specific beatmap
3 participants