-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Add UR estimation to the osu!mania ruleset #22613
base: pp-dev
Are you sure you want to change the base?
Conversation
Improves `maniastatacc` branch for Tests
Tests should be more manageable now. |
I've run an SR/PP spreadsheet for these changes, and attached it to the OP. Please have a look over it to make sure that the values are expected :) |
Sheet looks fine, I cross checked a few values and theyre all fine |
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.
LGTM
Will require reapproval due to #24636 |
Just to be safe... !diffcalc |
<ItemGroup> | ||
<PackageReference Include="MathNet.Numerics" Version="5.0.0" /> | ||
</ItemGroup> |
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.
How feasible is it to eliminate this mathnet reference in a way similar to #20963?
Yes I keep banging on about that and it's probably inconvenient but it matters if it's going to decrease binary size / dependency blast radius.
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.
Yes, tsunyoku ported the relevant functions to a file. I just have to clean up the implementation
@@ -12,6 +12,12 @@ public class ManiaPerformanceAttributes : PerformanceAttributes | |||
[JsonProperty("difficulty")] | |||
public double Difficulty { get; set; } | |||
|
|||
[JsonProperty("estimated_ur")] | |||
public double? EstimatedUr { get; set; } |
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.
Same as #20963, would prefer this to be called EstimatedUnstableRate
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.
Sure.
OverallDifficulty = values[ATTRIB_ID_OVERALL_DIFFICULTY]; | ||
NoteCount = (int)values[ATTRIB_ID_NOTE_COUNT]; | ||
HoldNoteCount = (int)values[ATTRIB_ID_HOLD_NOTE_COUNT]; |
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.
I was going to ask why these are not just being read from the IBeatmapOnlineInfo
param here and skipped entirely in .ToDatabaseAttributes()
, but then realised that wouldn't work because of keymods. I'm not sure how intentional this was but yeah.
This is going to be something that we need to be wary of going forward unless realtime calculation becomes a thing.
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 part in particular was added by smoogi to calculate a sheet for the rework
Deployment considerations:
Seemingly nothing further to point out from the infra side. |
@bdach Before you start any code structure reviews, what are your passing thoughts on the structure of https://github.com/Natelytle/osu/blob/maniastataccrefactor (specifically Utils/LogProb.cs)? I find it's much better for parsing the mathematics since the log stuff is abstracted away, but I'd like a second opinion. |
I'm not sure I'm going to be doing math/methodology correctness reviews on diffcalc anymore myself as I'm not sure that it's productive or even needed at this point. I was more angling to do just general code quality stuff. That said, since you asked directly, I'm not sure what to think of that thing. Some of the operations there have me worried about correctness of function domain and/or numerical stability - specifically I'm talking about stuff like presenting etc (assuming Again as per my opening post I'd probably not ding this in review because I'm not sure I care about the meat and potatoes of diffcalc changes at this point, but yeah. If these operations are meant to have some probabilistical interpretation then there should be xmldoc that explains what it is, otherwise it just doesn't seem like a very robust abstraction. Oh and if that thing is gonna stay then please change the name to something resembling full words, |
Massive thanks to Frost for the majority of the math behind this rework, and to Evening, Molneya, and Komirin for guidance.
Estimates Unstable Rate and replaces Accuracy with it
Changes
Reasoning
Estimation Theory
In order to estimate UR, we assume all hits are normally distributed, with a mean of ±0 and a deviation σ. This gives us the probability that with a certain σ, any given hit lands in a given window. We can compare these percentages to the true percentages of any given judgement in a play, and return whichever σ value is the closest match.
Further documentation can be found in a google doc here.
Considerations
SR/PP spreadsheet: https://docs.google.com/spreadsheets/d/1SBFrFOWIxJM-gACZrOk-2I6mH35MN9FRUDXbrTcGnJs/edit
As of a2197e2