Fix LotV apm #12
Fix LotV apm #12
Conversation
|
Added test for fix. |
|
|
||
| game_seconds_per_second = 1.4 | ||
| if replay.expansion == 'LotV': | ||
| game_seconds_per_second = 1.4 |
There was a problem hiding this comment.
it's weird to have an if branch that has no effect. game_seconds_per_second is 1.4 no matter what.
There was a problem hiding this comment.
Oh, that's unintentional - should have been 1 for LotV - will update it right away.
There was a problem hiding this comment.
It's weird that the test passed if the number was wrong. Did the test also have the wrong number? Or does the test not test this?
There was a problem hiding this comment.
I just took the output from the test and set it as expected, since I don't have a good understanding of what the avg_apm actually has to be because of rounding and series manipulation (the 0th minute is excluded for some reason).
I wasn't able to get it equal to in-game match screen without tweaking numbers and I didn't want to just tweak a number to multiply with without understanding why, so I tried to get it as close as possible while still understanding how. 😄
Should we try to get the same numbers as in-game (at least for average)?
There was a problem hiding this comment.
However, I did find the replay from the test in ggtracker.com and there the avg APM is 40 for the player, so the number 56 for LotV does make sense since 56==40*1.4. Might be a long shot though.
Replay is here (23 minute game):
http://ggtracker.com/players/1668928/Zenchii
There was a problem hiding this comment.
We have two issues now:
- is/was your new test affected by whether the number was 1.0 or 1.4? i.e. does the test test what it's supposed to test?
- how should APMs be handled / verified
Regarding 2), this PR should either not change APMs, or it should change APMs to get close to what people see in-game.
So, two ways to go:
a) run the old pre-this-PR code on this replay, see what APM comes out, and verify that this PR does not change the APM using your test
b) run the replay in the game client, see what APM is reported and see that our computed APM is in the same neighborhood
I'm happy either way. APM handling may be in a bad state now, but the point of this PR wasnt to fix everything about APM, but just to get the current chart to show up fully. But we shouldn't let an unintentional change to APM logic go through unless the change moves us closer to correctness.
There was a problem hiding this comment.
Yeah, you're right. The test was affected, so it is impacted by the change. I tried to fix it but now it just make even less sense to me. I think I need a second iteration on this to understand if we actually can separate APM calculation and APM visualization in two independent tasks.
Afaiu, APM is actions-per-game-minute, so it is affected by the LotV re-definition of a game-minute. A player has one fixed amount of actions in a replay, so shortening down the game-length of a replay increases the APM - both avg APM and the values of the points in the series in the APM chart. As you noted somewhere else, the 22 observations need to be fitted into the 15 timeslots.
There was a problem hiding this comment.
@nickelsen don't let logic get in the way of empirical examination. In particular, don't assume that Blizzard changed nothing in the meantime.
https://www.reddit.com/r/starcraft/comments/3wzhkg/310_patch_fixes_apm_sc2_now_shows_your_real_apm/
I believe we can separate APM calculation and visualization into two independent tasks. However you might find it easier to just fix APM match what Bliz says in-game than to try and keep the tasks separate.
There was a problem hiding this comment.
I just can't stand magic constants in programming, but maybe this is where I learn their usefulness. 😉
Meanwhile, I actually managed to produce avg apm equal to in-game from a recent replay, so now I just need the app to understand.
|
Closing for reasons stated here: |
Fix LotV apm by squeezing events into rescaled game minutes.