Skip to content

Fix LotV apm #12

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.

Sign up for GitHub

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

Closed
nickelsen wants to merge 3 commits into from
Closed

Fix LotV apm #12

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
604e629
Fix LotV apm by squeezing events into rescaled game minutes.
nickelsen Apr 21, 2016
d8fa276
Test for LotV apm fix.
nickelsen Apr 21, 2016
f762293
Move multipliers into constants and make calulations easier to read.
nickelsen Apr 21, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 13 additions & 4 deletions sc2reader/factories/plugins/replay.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,21 +100,30 @@ def APMTracker(replay):
above actions divided by the number of seconds played by the player (not
necessarily the whole game) multiplied by 60.
"""

speed_multiplier = 1
if replay.expansion == 'LotV':
speed_multiplier = 1.4
Copy link
Member

@dsjoerg dsjoerg Apr 21, 2016

Choose a reason for hiding this comment

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

how about consolidating all the 1.4s into a single place? i see them in sc2reader/engine/plugins/apm.py, sc2reader/factories/plugins/replay.py and sc2reader/resources.py


game_seconds_per_second = 1.4
if replay.expansion == 'LotV':
game_seconds_per_second = 1.4
Copy link
Member

@dsjoerg dsjoerg Apr 21, 2016

Choose a reason for hiding this comment

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

it's weird to have an if branch that has no effect. game_seconds_per_second is 1.4 no matter what.

Copy link
Contributor Author

@nickelsen nickelsen Apr 21, 2016

Choose a reason for hiding this comment

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

Oh, that's unintentional - should have been 1 for LotV - will update it right away.

Copy link
Member

@dsjoerg dsjoerg Apr 21, 2016

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@nickelsen nickelsen Apr 21, 2016

Choose a reason for hiding this comment

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

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)?

Copy link
Contributor Author

@nickelsen nickelsen Apr 21, 2016

Choose a reason for hiding this comment

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

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

Copy link
Member

@dsjoerg dsjoerg Apr 21, 2016

Choose a reason for hiding this comment

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

We have two issues now:

  1. 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?
  2. 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.

Copy link
Contributor Author

@nickelsen nickelsen Apr 21, 2016

Choose a reason for hiding this comment

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

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.

Copy link
Member

@dsjoerg dsjoerg Apr 21, 2016

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor Author

@nickelsen nickelsen Apr 21, 2016

Choose a reason for hiding this comment

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

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.


for player in replay.players:
player.aps = defaultdict(int)
player.apm = defaultdict(int)
player.seconds_played = replay.length.seconds

for event in player.events:
if event.name == 'SelectionEvent' or 'AbilityEvent' in event.name or 'Hotkey' in event.name:
player.aps[event.second] += 1.4
player.apm[int(event.second/60)] += 1.4
player.aps[event.second/speed_multiplier] += game_seconds_per_second
player.apm[int(event.second/60/speed_multiplier)] += game_seconds_per_second

elif event.name == 'PlayerLeaveEvent':
player.seconds_played = event.second
player.seconds_played = event.second/speed_multiplier

if len(player.apm) > 0:
player.avg_apm = sum(player.aps.values())/float(player.seconds_played)*60
player.avg_apm = sum(player.aps.values())/float(player.seconds_played)*60*speed_multiplier
else:
player.avg_apm = 0

Expand Down
11 changes: 11 additions & 0 deletions test_replays/test_all.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -494,6 +494,17 @@ def test_lotv_time(self):
self.assertEqual(replay.length.seconds, 1002)
self.assertEqual(replay.real_length.seconds, 1002)

def test_lotv_apm(self):
from sc2reader.factories.plugins.replay import APMTracker, SelectionTracker, toJSON
factory = sc2reader.factories.SC2Factory()
factory.register_plugin("Replay", APMTracker())
replay = factory.load_replay("test_replays/lotv/lotv1.SC2Replay")
for player in replay.players:
from pprint import pprint
pprint(player)
if player.name == 'Zenchii':
self.assertTrue(int(player.avg_apm) == 56)


class TestGameEngine(unittest.TestCase):
class TestEvent(object):
Expand Down