History class #1241

marcharper · 2019-02-22T03:03:26Z

Adds a History class, refactors Player a bit, and simplifies a number of tests and constructions. Note there were some subtle errors in the Meta players.

Increased runtime for a noisy tournament is ~25% but there are some optimizations possible:

refactoring strategies to avoid the unnecessary property wrappers on the Player class
Using a single joint history (adding the opponent's cooperations and defections) per Match
Adding a version of the History class that doesn't track the state distributions and such when not needed (could detect with classifiers)
Ultimately I'd like to change Player to only take a history object rather than the full opponent object

Also, I believe we could further refactor the dual transformer to be more efficient now but I didn't try that yet.

Updates:

Added a LimitedHistory class that only retains N rounds of history, which improves the memory_depth test
Removed nearly all instances of direct history assignment on the Player class, which should always be discouraged IMO
Made various related minor updates
I changed the meta classes so that each player on the meta team tracks its own history. Previously they were not updated with the history and so may not have properly set their internal variables properly as play proceeds. This PR has caused or exposed the issue.

I have fixed the remaining issues:

the generators were not equal because the meta class was updating team histories on the next round, so the generators were off by one. I introduced an update_history method to the Player and MetaPlayer classes to fix, this method allows "post processing" of the moves by the players to fully end the round.
transformed SequencePlayers could not be pickled (there was no test). They can now and there are various new tests. I ended up adding a __getstate__ method to Player (overridden in SequencePlayer and needed an abstract version in Player).

Tournaments take about 25% longer overall. In addition to the tasks in the first comment, we can get some or all of that back by optimizing in a subsequent PR or on the 5.0.0 branch:

removing unnecessary function calls: inlining simultaneously_play, for example, and potentially optimizing how the history class is used in various places
revisiting how the tournament class marshalls players

Increased runtime for the full standard tournament is 15% (30 hours versus 26 on my desktop). I think we'll easily get that back since removing function call overhead in #1239 yielded an improvement of ~25%.

…rategy tests

marcharper · 2019-02-22T05:53:47Z

The pickle issue has something to do with the MetaPlayer having a sequence player in the team (ThueMorse or ThueMorseInverse). When unpickled the subplayer isn't equal to itself, but its own test passes...

drvinceknight · 2019-02-22T18:08:44Z

Sounds interesting @marcharper, I'll try and get to this next week. 👍

drvinceknight

Some minor comments after an initial read through, it all looks good to me though 👍

I haven't had a go at debugging the failing test, I'll try and get to that when I get a bit more time :)

axelrod/history.py

drvinceknight · 2019-02-26T09:28:24Z

axelrod/history.py

+            return self._plays == other._plays and self._coplays == other._coplays
+        raise TypeError("Cannot compare types.")
+
+    def __getitem__(self, key):


Just a comment: This could be useful if we want to incorporate misperception (as opposed to noise). 👍

axelrod/player.py

axelrod/random_.py

drvinceknight · 2019-02-26T09:37:28Z

axelrod/tests/strategies/test_backstabber.py

@@ -172,101 +167,3 @@ def test_alt_strategy_stops_after_round_180(self):
            expected_actions=expected_actions,
            match_attributes={"length": 200},
        )
-
-
-class TestModuleMethods(unittest.TestCase):


Why can we remove all of these?

Good question. Some are just testing Player class properties that already have their own tests. But it looks like I was a bit overzealous.

Actually I think all of these tests are redundant. If we want to keep any I'd prefer we re-write them to use an actual match rather than simply overwriting the history, which is a practice that we shouldn't encourage.

Yeah I agree about not encouraging using history overwriting, could we either rewrite as part of this PR or leave them in and open an issue to remove them?

I'll check but I think these tests are just directly targeting the subfuctions of the strategy and are all covered by the "standard" tests above in the file, so rewriting them would produce near identical tests

axelrod/history.py

axelrod/strategies/meta.py

marcharper · 2019-02-27T07:16:12Z

Updates:

Added a LimitedHistory class that only retains N rounds of history, which improves the memory_depth test
Removed nearly all instances of direct history assignment on the Player class, which should always be discouraged IMO
Made various related minor updates

I suspect the pickle errors have something to do with the decorated meta classes and how they are rebuilt. I changed the meta classes so that each player on the meta team tracks its own history. Previously they were not updated with the history and so may not have properly set their internal variables properly as play proceeds. This PR has caused or exposed the issue.

marcharper · 2019-03-04T04:36:19Z

I'm still not sure why the meta players with sequence players on their teams don't pass the pickle test. (It's sufficient to use just ThueMorse as the team.) When you examine the players on team you see this:

{'init_kwargs': OrderedDict(),
'_history': [C, D, D, C, D, C, C, D, D],
'classifier': {'memory_depth': inf, 'stochastic': False, 'makes_use_of': set(), 
'long_run_time': False, 'inspects_source': False, 'manipulates_source': False, 'manipulates_state': False}, 
'match_attributes': {'length': -1, 'game': Axelrod game: (R,P,S,T) = (3, 1, 0, 5), 'noise': 0}, 
'sequence_generator': <generator object Player.__eq__.<locals>.<genexpr> at 0x7fc6aef72c50>}

{'init_kwargs': OrderedDict(),
'_history': [C, D, D, C, D, C, C, D, D],
'classifier': {'memory_depth': inf, 'stochastic': False, 'makes_use_of': set(),
'long_run_time': False, 'inspects_source': False, 'manipulates_source': False, 'manipulates_state': False}, 
'match_attributes': {'length': -1, 'game': Axelrod game: (R,P,S,T) = (3, 1, 0, 5), 'noise': 0}, 
'sequence_generator': <generator object Player.__eq__.<locals>.<genexpr> at 0x7fc6aef72b48>}

Note that the sequence_generator attribute seem to correlate the instances when passed through the Player.__eq__ function .

Oddly enough when I add similar tests to the TestPlayer I don't get the same failure.

marcharper · 2019-03-04T04:40:42Z

Also note that I made a significant change to the metaplayers in that they now pass along the team member's individual play choices rather than all sharing a single history. I think this makes more sense but it will impact how they play.

drvinceknight · 2019-03-15T18:50:12Z

Also note that I made a significant change to the metaplayers in that they now pass along the team member's individual play choices rather than all sharing a single history. I think this makes more sense but it will impact how they play.

Yeah I'm fine with this change, they'll still always be looking at the correct history for the opponent. 👍

drvinceknight · 2019-03-15T18:59:16Z

Note that the sequence_generator attribute seem to correlate the instances when passed through the Player.__eq__ function .

Oddly enough when I add similar tests to the TestPlayer I don't get the same failure.

I've started to have a look (really sorry it's taking me so long to try and be helpful). The __eq__ method in the player class needs to be a bit smart with the equality of a sequence, see https://github.com/Axelrod-Python/Axelrod/blob/master/axelrod/player.py#L145 where it essentially needs to split the generator in to two copies of itself. Use one of the copies to check equality and then set the attribute with the other copy.

I'm not entirely sure why this would fall over when checking pickling equality but perhaps that's something worth considering? (If you haven't already).

…ayer (still broken)

marcharper · 2019-03-19T06:28:03Z

I believe I have fixed the remaining issues:

the generators were not equal because the meta class was updating team histories on the next round, so the generators were off by one. I introduced an update_history method to the Player and MetaPlayer classes to fix, this method allows "post processing" of the moves by the players to fully end the round.
transformed SequencePlayers could not be pickled. They can now and there are various new tests. I ended up adding a __getstate__ method to Player (overridden in SequencePlayer and needed an abstract version in Player).

Tournaments take about 25% longer overall. In addition to the tasks in the first comment, we can get some or all of that back by optimizing in a subsequent PR or on the 5.0.0 branch:

removing unnecessary function calls: inlining simultaneously_play, for example, and potentially optimizing how the history class is used in various places
revisiting how the tournament class marshalls players -- there's no need to rebuild them now (if more efficient)

drvinceknight · 2019-03-19T08:36:42Z

Nice work!

Tournaments take about 25% longer overall. In addition to the tasks in the first comment, we can get some or all of that back by optimizing in a subsequent PR or on the 5.0.0 branch:

Is there any merit in me running a full tournament to see if the 25% increase scales well?

marcharper · 2019-03-19T14:28:48Z

Is there any merit in me running a full tournament to see if the 25% increase scales well?

Yes, and we should take a look at the impact on the Meta strategies

drvinceknight · 2019-03-19T15:50:26Z

Yes, and we should take a look at the impact on the Meta strategies

Ok cool, I'll aim to get some things running for that.

drvinceknight · 2019-03-21T19:38:31Z

I've got some code running. Will report back once it's done 👍

marcharper · 2019-03-23T16:09:25Z

Increased runtime for the full standard tournament is 15% (30 hours versus 26 on my desktop). I think we'll easily get that back since removing function call overhead in #1239 yielded an improvement of ~25%.

marcharper · 2019-03-26T01:50:56Z

@drvinceknight @meatballs I think this is good to go in, PTAL when you get a chance.

drvinceknight · 2019-03-26T22:41:48Z

I agree it's pretty much there (sorry for being so slow to get back to you, I've got a tonne of marking at the moment), I've got some long (probably too long) tournaments running for timing experiments but we can merge and if they come back atrocious (which I doubt they will) we can always figure that out later.

I've already looked through and everything looks good to me, but a close look from @meatballs would be great 👍

marcharper · 2019-04-05T02:33:01Z

@meatballs PTAL

marcharper added 8 commits February 20, 2019 20:04

Add History classes

287cbda

History class updates

f6a1f0e

Some updates to the History list class

0d87091

Update history class and tests

e364405

Update Player class and tests for History class changes

4f02e9a

Update transformers, tests, and add dual history

66dadcc

Cleanup Player and History classes

8ffc6de

Adjust some strategies and tests to submit a coplay; fix some Meta st…

18ee8d9

…rategy tests

marcharper added 4 commits February 21, 2019 22:02

Fix memory depth for Grudger and Meta tests after Gruder correction

1f0a6e1

Update doc test counts

befe0b9

Cover lines missing tests

e1af177

More updates to meta classes and tests

247dca7

marcharper force-pushed the history-class-2019 branch from ab90fdb to 247dca7 Compare February 22, 2019 06:02

Fix test collision name and extend sequence player pickling test

18d3950

marcharper changed the title ~~History class 2019~~ History class Feb 23, 2019

drvinceknight requested changes Feb 26, 2019

View reviewed changes

marcharper commented Feb 27, 2019

View reviewed changes

axelrod/strategies/meta.py Outdated Show resolved Hide resolved

marcharper added 2 commits February 26, 2019 19:50

Various minor updates requested in review

fb31a5b

Eliminate most instances of direct history assignment

c9c018b

marcharper added 2 commits February 26, 2019 23:34

Change memory depth test to use the max memory depth of the two players

0d31529

Update some tests and add new tests

b8cdc5e

marcharper added 2 commits March 18, 2019 22:30

Fix meta sequence player pickling, add test for decorated sequence pl…

5a6a958

…ayer (still broken)

Fix pickling for decorated sequence players

ec1bbf9

marcharper added the ready-for-review label Mar 19, 2019

Cleanup a few lines no longer needed and update history tests

f49abcc

marcharper mentioned this pull request Mar 23, 2019

Possibly move play method away from Player #1181

Closed

drvinceknight approved these changes Mar 26, 2019

View reviewed changes

meatballs merged commit 2349c49 into master Apr 5, 2019

meatballs deleted the history-class-2019 branch April 5, 2019 08:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

History class #1241

History class #1241

marcharper commented Feb 22, 2019 •

edited

Loading

marcharper commented Feb 22, 2019

drvinceknight commented Feb 22, 2019

drvinceknight left a comment

drvinceknight Feb 26, 2019

marcharper Feb 27, 2019

drvinceknight Feb 26, 2019

marcharper Feb 27, 2019

marcharper Feb 27, 2019

drvinceknight Feb 27, 2019

marcharper Feb 27, 2019 •

edited

Loading

marcharper commented Feb 27, 2019 •

edited

Loading

marcharper commented Mar 4, 2019 •

edited

Loading

marcharper commented Mar 4, 2019

drvinceknight commented Mar 15, 2019

drvinceknight commented Mar 15, 2019

marcharper commented Mar 19, 2019 •

edited

Loading

drvinceknight commented Mar 19, 2019

marcharper commented Mar 19, 2019

drvinceknight commented Mar 19, 2019

drvinceknight commented Mar 21, 2019

marcharper commented Mar 23, 2019 •

edited

Loading

marcharper commented Mar 26, 2019

drvinceknight commented Mar 26, 2019

marcharper commented Apr 5, 2019

History class #1241

History class #1241

Conversation

marcharper commented Feb 22, 2019 • edited Loading

marcharper commented Feb 22, 2019

drvinceknight commented Feb 22, 2019

drvinceknight left a comment

Choose a reason for hiding this comment

drvinceknight Feb 26, 2019

Choose a reason for hiding this comment

marcharper Feb 27, 2019

Choose a reason for hiding this comment

drvinceknight Feb 26, 2019

Choose a reason for hiding this comment

marcharper Feb 27, 2019

Choose a reason for hiding this comment

marcharper Feb 27, 2019

Choose a reason for hiding this comment

drvinceknight Feb 27, 2019

Choose a reason for hiding this comment

marcharper Feb 27, 2019 • edited Loading

Choose a reason for hiding this comment

marcharper commented Feb 27, 2019 • edited Loading

marcharper commented Mar 4, 2019 • edited Loading

marcharper commented Mar 4, 2019

drvinceknight commented Mar 15, 2019

drvinceknight commented Mar 15, 2019

marcharper commented Mar 19, 2019 • edited Loading

drvinceknight commented Mar 19, 2019

marcharper commented Mar 19, 2019

drvinceknight commented Mar 19, 2019

drvinceknight commented Mar 21, 2019

marcharper commented Mar 23, 2019 • edited Loading

marcharper commented Mar 26, 2019

drvinceknight commented Mar 26, 2019

marcharper commented Apr 5, 2019

marcharper commented Feb 22, 2019 •

edited

Loading

marcharper Feb 27, 2019 •

edited

Loading

marcharper commented Feb 27, 2019 •

edited

Loading

marcharper commented Mar 4, 2019 •

edited

Loading

marcharper commented Mar 19, 2019 •

edited

Loading

marcharper commented Mar 23, 2019 •

edited

Loading