New meta strategies #777

marcharper · 2016-12-03T08:41:13Z

Several new meta strategies
Reworked the meta tests significantly (much simpler now I hope)
~~Caught some other classification related issues, and a cache issue that I haven't yet addressed (see Cache Bug Lurking #779)~~ now fixed

drvinceknight · 2016-12-03T15:53:04Z

There are a number of tests failing (that at a glance look genuine) but I took a quick look at why the docs weren't building.

It seems to be because we mock numpy (we do this so that there are no install problems on readthedocs) in the docs/conf.py file but one of your strategies uses numpy.random.choice. This is failing at the import axelrod stage of docs/conf.py.

I'm not entirely sure why this is breaking for this particular strategy (we have other strategies that make use of numpy.random.choice. Let me know if you need more of a hand with this and I can look in to it more.

marcharper · 2016-12-03T19:44:05Z

I'm working on the tests... I need to rerun the strategy timing notebook to check the long run time values.

One of the new strategies, MWERandom, is going to be hard to test as fully since it picks a random team each time.

drvinceknight · 2016-12-03T19:44:39Z

One of the new strategies, MWERandom, is going to be hard to test as fully since it picks a random team each time.

Yeah I thought that. As long as the lines get hit and we have some cases covered I'm happy...

marcharper · 2016-12-03T21:09:23Z

The issue with MWERandom is that it ends up in a classifier filter test and it's classifier changes based on the instantiation. So while the classification is correct, the test that compares the two strategy sets fails randomly if the teams cause different classifiers in the meta strategy.

marcharper · 2016-12-04T02:08:25Z

Help on the last two tests is appreciated. I suspect one is due to the MetaWinner reset bug I fixed and the other is MWERandom, we'll need a strategy to exclude it somehow.

drvinceknight · 2016-12-04T15:24:04Z

Help on the last two tests is appreciated.

I've pushed some code to new_meta-vk.

I've fixed the filterset test, not by dropping MWERandom out of the test but by seeding both filtering approaches (I made this a hypothesis test to randomly pick the seed):

09cd7ad tweaks how the filterset was doing this. It was previously initialising a player for each property. Now it just initialises a player once.
483b485 seeds the two approaches in the test.

I'm just going to take a look at the other failing test now :)

EDIT: At some stage I thought that my fix on #782 would have helped with this so my new_meta-vk branch is rebased on top of that. Depending on what you choose to do it might be worth just cherry-picking the commits.

drvinceknight · 2016-12-04T15:39:41Z

I suspect one is due to the MetaWinner reset bug I fixed

I didn't quite follow what the bug was and what the fix is. Looks like you've removed the reset method from adaptive. What's the reason?

marcharper · 2016-12-04T16:13:52Z

Adaptive still has its reset method, I just thought it was bad form to call it in the constructor.

The MetaWinner bug was that the cumulative scores were being recorded in the attributes of the players in the team and not being reset. This tripped your test on repeated deterministic matches for MetaWinnerDeterministic (being the only deterministic meta strategy) and has gone unnoticed for the other MetaWinner variants until now. In other words retaining state made the outcomes not repeatable.

The fix is reseting the scores in MetaWinner's reset function, and making the test reset method better to try to prevent this in the future.

marcharper · 2016-12-04T16:28:47Z

The second commit doesn't want to cherry-pick, if you're able to do it feel free to push directly to this branch.

drvinceknight · 2016-12-04T17:51:29Z

The second commit doesn't want to cherry-pick, if you're able to do it feel free to push directly to this branch.

I've just pushed that. Looking at the second test now :)

drvinceknight · 2016-12-04T18:16:00Z

Reverting 8128e72 didn't seem to affect that test.

I've fixed it with: b97d413, this just sets the seed before the play of the tournament. I'm not entirely sure why that is required here.

drvinceknight · 2016-12-04T18:33:05Z

b157d91 fixes the doc build.

This is actually an interesting precedent. MWERandom is going to be the first strategy whose classification is random on some dimensions.

This triggered problems with the doc build:

The doc build imports axelrod (to be able to autodocument the strategies);
To be able to use readthedocs we have mocked a variety of modules, including numpy;
During the import of axelrod a bunch of filtered strategy sets are created which requires strategies to be initialised (including MWERandom).
This was breaking when hitting numpy.choice in MWERandom's init function.

I swapped numpy.choice for random.sample.

marcharper · 2016-12-04T19:06:43Z

I've fixed it with: b97d413, this just sets the seed before the play of the tournament. I'm not entirely sure why that is required here.

Maybe because MWERandom uses random values in its initialization?

drvinceknight · 2016-12-04T19:11:22Z

The docs are failing to build, I'm taking a look at that

Maybe because MWERandom uses random values in its initialization?

I thought that but the way it was before was seeding after creating the players... Although maybe something I'm forgetting in the tournament initialization was initializing MWERandom again... Although we were seeding before both tournaments initialised, not sure I'd need to think about that one but my head is pretty dead right now :)

drvinceknight · 2016-12-04T19:16:08Z

@marcharper I've merged in #782 so perhaps that will let you tweak some of the fixes you have here?

They used to take a strategy class but this ended up initialising players multiples times.

Using numpy causes problems for building the docs. Building the docs mocks numpy (I believe this is because readthedocs can't handle numpy), which in turn broke when importing axelrod (for the auto documentation) as various filtered lists are created when importing. MWE Random is the first strategy that initialises and classifies itself stochasticly...

marcharper · 2016-12-04T19:24:06Z

Ok I removed the classification workaround and all the tests passed locally. I also rebased onto the new master (which required a force push).

drvinceknight

~~This looks ok to me. I've left a couple of questions more than anything else (and 1 minor change to a comment that I should have done in one of my commits).~~

I pushed the tiny fix to the inline comment myself: 07caf8f

Changing this to a positive review from me (my other two comments are just questions).

drvinceknight · 2016-12-05T08:19:47Z

axelrod/deterministic_cache.py

@@ -99,7 +99,7 @@ def _is_valid_key(self, key):
        if len(key) != 3:
            return False

-        # The triplet should be a pair of axelrod.Player sublclasses and an
+        # The triplet should be a pair of axelrod.Player subclasses and an


This has been changed: should be axelrod.Player instances

drvinceknight · 2016-12-05T08:20:33Z

axelrod/strategies/__init__.py

-                       MetaMajorityFiniteMemory, MetaWinnerFiniteMemory,
-                       MetaMajorityLongMemory, MetaWinnerLongMemory, MetaMixer,
-                       MetaWinnerEnsemble])
+all_strategies += [MetaHunter,


Is this just a stylistic change?

(I don't mind at all, not requesting a change here, just curious.)

drvinceknight · 2016-12-05T08:22:30Z

axelrod/strategies/meta.py

@@ -37,11 +45,12 @@ def __init__(self):
        self.team = [t() for t in self.team]

        # This player inherits the classifiers of its team.
+        # Note that memory_depth is not simply the max memory_depth of the team.


Really, why not?

For e.g. MetaWinner it's saving the cumulative scores of each strategy even if the strategies themselves are not.

meatballs · 2016-12-05T08:42:54Z

The filtering was being done at class level in order for the API to work - instances aren't available at the point it's used.

Have the changes here stopped the filtering working on a class?

drvinceknight · 2016-12-05T08:48:34Z

Have the changes here stopped the filtering working on a class?

No, they still work on a class, the main filtering function passes_filterset takes a class. The change is that the sub functions (that passes_filterset call) you had written which also took the class now take a player: in other once we're only creating a player instance once (as opposed to creating an instance for each property being checked).

FYI This is the commit that implemented it: e9273ef and no actual tests were changed for this.

drvinceknight · 2016-12-05T09:03:24Z

This does point to another slightly sticky problem with MWERandom and the fact that when initiated it can be take a random classification (this strategy sets an interesting precedent for that so let's try to get it right).

I think I would prefer if the classification should just be hard coded to take extreme values: memory_length=inf, makes_use_of['game']=True etc... That's the potential memory_length it can have...

My main thinking is reproducibility... If we wanted MWERandom in an experiment and looked at scores vs memory length then we'd get different results.

drvinceknight · 2016-12-05T09:06:54Z

My main thinking is reproducibility... If we wanted MWERandom in an experiment and looked at scores vs memory length then we'd get different results.

We could of course seed, but we're then talking about seeding every single init of the strategy (for example inside the tournament play itself!).

I think my current thoughts would suggest that as a general rule of thumb, classification should be 'set' by the input parameters (if any). So for example Random(0) is deterministic, and MWERandom() should have infinite memory length.

Cancelling: see comment about classification of MWERandom.

marcharper · 2016-12-05T12:57:29Z

We can remove MWERandom from the default set of strategies, I'm not super attached to it. But it did help reveal some issues as I'm sure you've noticed.

drvinceknight · 2016-12-05T14:50:28Z

But it did help reveal some issues as I'm sure you've noticed.

Absolutely! Found some great stuff :)

Would still be nice to have in, just a question of classifying it. I'm going to remove it for now and push.

drvinceknight · 2016-12-05T15:19:04Z

I've removed MWERandom. Giving this a 👍 from me.

marcharper force-pushed the new_meta branch from 484faf5 to 6691466 Compare December 3, 2016 20:43

marcharper added the ready-for-review label Dec 4, 2016

marcharper and others added 13 commits December 4, 2016 11:19

New meta strategies

885663f

Fix a number of Meta Tests, add MetaHunterAggressive

b986de2

Rework Meta tests

665a566

Fix more tests around caching and classification

3d5d85a

MetaWinnerDeterministic and MetaWinnerStochastic

6ab6b72

Workaround for #779

e905c7b

Remove MWEFast

923af2c

Adjust long run time classifications

c10c1aa

Fix MetaWinner reset bug

741cd5f

Improve test_reset

434eeff

Comments

5a19e3e

Add message to test assertion.

2f4443a

Seed the classification in the test.

f764fc5

drvinceknight and others added 6 commits December 4, 2016 11:19

Change sub function for filterset to take player.

e9273ef

They used to take a strategy class but this ended up initialising players multiples times.

Remove unneeded import.

94a13b1

Seed the play of the tournaments.

a6cd346

Adjust the classification docs.

8f5c7d0

Remove workaround for #779

2b5ec7c

marcharper force-pushed the new_meta branch from 02a5211 to 2b5ec7c Compare December 4, 2016 19:23

drvinceknight requested changes Dec 5, 2016

View reviewed changes

Change inline comment.

07caf8f

drvinceknight previously approved these changes Dec 5, 2016

View reviewed changes

drvinceknight added the ready-to-merge label Dec 5, 2016

drvinceknight removed the ready-to-merge label Dec 5, 2016

Remove MWERandom.

c95a829

drvinceknight approved these changes Dec 5, 2016

View reviewed changes

drvinceknight added the ready-to-merge label Dec 5, 2016

meatballs merged commit e0c8b75 into master Dec 6, 2016

meatballs deleted the new_meta branch December 6, 2016 08:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New meta strategies #777

New meta strategies #777

marcharper commented Dec 3, 2016 •

edited

Loading

drvinceknight commented Dec 3, 2016

marcharper commented Dec 3, 2016

drvinceknight commented Dec 3, 2016

marcharper commented Dec 3, 2016

marcharper commented Dec 4, 2016 •

edited

Loading

drvinceknight commented Dec 4, 2016 •

edited

Loading

drvinceknight commented Dec 4, 2016

marcharper commented Dec 4, 2016 •

edited

Loading

marcharper commented Dec 4, 2016

drvinceknight commented Dec 4, 2016

drvinceknight commented Dec 4, 2016

drvinceknight commented Dec 4, 2016

marcharper commented Dec 4, 2016

drvinceknight commented Dec 4, 2016

drvinceknight commented Dec 4, 2016

marcharper commented Dec 4, 2016

drvinceknight left a comment •

edited

Loading

drvinceknight Dec 5, 2016

drvinceknight Dec 5, 2016

marcharper Dec 5, 2016

drvinceknight Dec 5, 2016

marcharper Dec 5, 2016

meatballs commented Dec 5, 2016

drvinceknight commented Dec 5, 2016

drvinceknight commented Dec 5, 2016

drvinceknight commented Dec 5, 2016

marcharper commented Dec 5, 2016

drvinceknight commented Dec 5, 2016

drvinceknight commented Dec 5, 2016

New meta strategies #777

New meta strategies #777

Conversation

marcharper commented Dec 3, 2016 • edited Loading

drvinceknight commented Dec 3, 2016

marcharper commented Dec 3, 2016

drvinceknight commented Dec 3, 2016

marcharper commented Dec 3, 2016

marcharper commented Dec 4, 2016 • edited Loading

drvinceknight commented Dec 4, 2016 • edited Loading

drvinceknight commented Dec 4, 2016

marcharper commented Dec 4, 2016 • edited Loading

marcharper commented Dec 4, 2016

drvinceknight commented Dec 4, 2016

drvinceknight commented Dec 4, 2016

drvinceknight commented Dec 4, 2016

marcharper commented Dec 4, 2016

drvinceknight commented Dec 4, 2016

drvinceknight commented Dec 4, 2016

marcharper commented Dec 4, 2016

drvinceknight left a comment • edited Loading

Choose a reason for hiding this comment

drvinceknight Dec 5, 2016

Choose a reason for hiding this comment

drvinceknight Dec 5, 2016

Choose a reason for hiding this comment

marcharper Dec 5, 2016

Choose a reason for hiding this comment

drvinceknight Dec 5, 2016

Choose a reason for hiding this comment

marcharper Dec 5, 2016

Choose a reason for hiding this comment

meatballs commented Dec 5, 2016

drvinceknight commented Dec 5, 2016

drvinceknight commented Dec 5, 2016

drvinceknight commented Dec 5, 2016

marcharper commented Dec 5, 2016

drvinceknight commented Dec 5, 2016

drvinceknight commented Dec 5, 2016

marcharper commented Dec 3, 2016 •

edited

Loading

marcharper commented Dec 4, 2016 •

edited

Loading

drvinceknight commented Dec 4, 2016 •

edited

Loading

marcharper commented Dec 4, 2016 •

edited

Loading

drvinceknight left a comment •

edited

Loading