Skip to content

Conversation

marcharper
Copy link
Member

It is necessary to instantiate strategies to determine the proper classifiers since __init__ can overwrite them. This PR has the following changes:

  • Handles the above comment
  • Imports strategies explicitly in _strategies.py
  • Adds a dynamically generated basic_strategies list
  • Moves some of the strategy creation logic to strategies/__init__.py. This is necessary because instantiating the Meta strategies creates a circular dependency, so they have to be appended afterward.
  • Updates a few of the classifier dictionaries and moves a few strategies around
  • Some minor updates to the docs and various PEP8

Copy link
Member

Choose a reason for hiding this comment

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

Does this still give the same set of strategies as before?

Copy link
Member

Choose a reason for hiding this comment

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

Could we include a test for it similar to the one for is_cheater? https://github.com/Axelrod-Python/Axelrod/blob/master/axelrod/tests/unit/test_classification.py#L54

@drvinceknight
Copy link
Member

This all looks good to me, just the one comment about the basic_strategies.

@marcharper
Copy link
Member Author

basic_strategies is bit bigger now:

        known_basic = [axelrod.Alternator,
                       axelrod.AntiTitForTat,
                       axelrod.Bully,
                       axelrod.Cooperator,
                       axelrod.CyclerCCCCCD,
                       axelrod.CyclerCCCD,
                       axelrod.CyclerCCD,
                       axelrod.Defector,
                       axelrod.GoByMajority,
                       axelrod.SuspiciousTitForTat,
                       axelrod.TitForTat,
                       axelrod.WinStayLoseShift]

I think this makes sense though, Bully, SuspiciousTitForTat, and AntiTitForTat are very similar to TitForTat (as is WSLS); Alternator would be CyclerCD.

@drvinceknight
Copy link
Member

Cool, thanks for the test and I'm happy with the new basic strategies but now the docs need updating - there's that example with basic_strategies... Again the docs need a whole revamp I think and a lot of tlc but if you could change it to something that was not wrong with the new code?

@marcharper
Copy link
Member Author

The new scores are a bit ugly, I'm not sure if they were rounded the last time around.

@langner
Copy link
Member

langner commented Sep 15, 2015

I liked the simplicity of the previous four basic strategies.

On Tue, Sep 15, 2015 at 8:56 AM, Marc Harper, PhD notifications@github.com
wrote:

The new scores are a bit ugly, I'm not sure if they were rounded the last
time around.


Reply to this email directly or view it on GitHub
#296 (comment)
.

@drvinceknight
Copy link
Member

I liked the simplicity of the previous four basic strategies.

I thought the same but did like the simplicity of the creation of the new ones. Perhaps creating another (loath to suggest this) list which is example_strategies or something which really is to just create an example tournament?

@drvinceknight
Copy link
Member

Ultimately the question is: what are the basic_strategies for? If it's to be able to create a simple example tournament to just show the basics of everything then I think the status quo was the way to go.

@langner
Copy link
Member

langner commented Sep 15, 2015

Well yeah, that's why it was called basic :) it's like the smallest
possible interesting tournament. Can also be used as a quick sanity check.
But it's OK, I understand why the paradigm has changed... the library
itself has grown and become quite complex now.

On Tue, Sep 15, 2015 at 12:07 PM, Vince Knight notifications@github.com
wrote:

Ultimately the question is: what are the basic_strategies for? If it's to
be able to create a simple example tournament to just show the basics of
everything then I think the status quo was the way to go.


Reply to this email directly or view it on GitHub
#296 (comment)
.

@marcharper
Copy link
Member Author

Actually this list isn't correct -- the cycler strategies have memory_depth greater than 1 and are misclassified. Let me fix this, and check the others.

@marcharper
Copy link
Member Author

Ok, now the list is:

Alternator
Anti Tit For Tat
Bully
Cooperator
Defector
Soft Go By Majority
Suspicious Tit For Tat
Tit For Tat
Win-Stay Lose-Shift

We could bump GoByMajority by excluding the default case (memory_depth 0, which is somewhat trivial). There are already multiple derivatives with depth 5, 10, 20, and 40.

@marcharper
Copy link
Member Author

@meatballs Can you please check that the tournament_factory tests and run_axelrod still make sense now that basic_strategies is a subset of ordinary strategies?

@drvinceknight
Copy link
Member

I thought basic_strategies being a subset was a non issue? I think someone
pointed that out to me... I might be wrong.

Re the actual list: I've thought about it more. While I like the simpler
list more (without the cyclers), I do wonder if this was a problem that
needed fixing. The only time someone would use basic_strategies is for a
quick demo, anything more complicated and the user could use the classifier
dictionary itself.

The slight issue I have with the new basic_strategies is that that demo is
not as simple as it used to be: explaining what each strategy does takes
much longer than before. The other issue I have with basic_strategies being
defined dynamically is that it is conceivable that this list would change
and expand which again goes away from it's purpose as a demo list.

In that vain, perhaps creating the same old list but calling it
demo_strategies is a good alternative? But then in that case I'm not sure
that basic_strategies would have a purpose...

Having said all that, I'm not terribly against the change. Although for
demo purposes I think it's important to have that old list quickly
available - the new list does not work as a good demo list. If this gets
merged as in before pyconuk I will not be using this list for my demo.

I'm just voicing where I stand, if the new list becomes popular: fine but
ultimately what is the basic_strategies list for - I understand what the
purpose of the old list is, I am not sure what the purpose of the new one
is, if we have a demo_strategies a long side it perhaps that's the best
alternative...

Heading to bed :)

On Tue, 15 Sep 2015 21:46 Marc Harper, PhD notifications@github.com wrote:

@meatballs https://github.com/meatballs Can you please check that the
tournament_factory tests and run_axelrod still make sense now that
basic_strategies is a subset of ordinary strategies?


Reply to this email directly or view it on GitHub
#296 (comment)
.

@marcharper
Copy link
Member Author

I guess I don't understand what the issue is -- seems to me like you could just do:

import axelrod
strategies = [axelrod.Alternator(), axelrod.Cooperator(), axelrod.Defector(), axelrod.Random(), axelrod.Tit For Tat()]
for s in strategies:
    print s

In any case, I don't really care much about basic_strategies, and if you all want to revert it to a static list that's fine with me. It was just on the todo list of #294, so I did it. Some of the other changes I introduced are important though (some strategies were not classified properly, and they need to be instantiated before they are filtered, so the movement of the meta strategies is also necessary).

For the record, I don't see why any of the strategies other than GoByMajority are hard to explain (and I recommended removing that one above). The others essentially do something very similar to TFT, e.g. defecting initially instead.

If someone does want to update the usage docs with a different list, here is the script I used to make it easier:

import axelrod


def find_number_of_tit_for_tat(number_of_defectors):
    strategies = [s() for s in axelrod.basic_strategies]
    for d in range(number_of_defectors - 1):
        strategies.append(axelrod.Defector())
    ranks = ['Defector']  # Creating a dummy list to start
    count = 1
    while ranks[0] == 'Defector':
        count += 1
        strategies.append(axelrod.TitForTat())
        tournament = axelrod.Tournament(strategies)
        results = tournament.play()
        ranks = results.ranked_names
    return count

strategies = [s() for s in axelrod.basic_strategies]
for s in strategies:
    print(s)

strategies.append(axelrod.Defector())

tournament = axelrod.Tournament(strategies)

print(tournament.players)

results = tournament.play()

print(results.normalised_scores)

print(results.ranking)

print(results.ranked_names)

while results.ranked_names[0] == 'Defector':
    strategies.append(axelrod.TitForTat())  # Adding a new tit for tat player
    tournament = axelrod.Tournament(strategies)
    results = tournament.play()
    ranks = results.ranked_names

print(results.ranked_names.count('Tit For Tat'))

d = range(2, 50)
t = [find_number_of_tit_for_tat(n) for n in d]

print(max(t))

@drvinceknight
Copy link
Member

Yeah completely agree about everything else - this is a valuable PR. I
would rather revert the basic strategies though.

On Tue, 15 Sep 2015 23:04 Marc Harper, PhD notifications@github.com wrote:

I guess I don't understand what the issue is -- seems to me like you could
just do:

import axelrod
strategies = [axelrod.Alternator(), axelrod.Cooperator(), axelrod.Defector(), axelrod.Random(), axelrod.Tit For Tat()]
for s in strategies:
print s

In any case, I don't really care much about basic_strategies, and if you
all want to revert it to a static list that's fine with me. It was just on
the todo list of #294
#294, so I did it. Some
of the other changes I introduced are important though (some strategies
were not classified properly, and they need to be instantiated before they
are filtered, so the movement of the meta strategies is also necessary).

For the record, I don't see why any of the strategies other than
GoByMajority are hard to explain (and I recommended removing that one
above). The others essentially do something very similar to TFT, e.g.
defecting initially instead.

If someone does want to update the usage docs with a different list, here
is the script I used to make it easier:

import axelrod

def find_number_of_tit_for_tat(number_of_defectors):
strategies = [s() for s in axelrod.basic_strategies]
for d in range(number_of_defectors - 1):
strategies.append(axelrod.Defector())
ranks = ['Defector'] # Creating a dummy list to start
count = 1
while ranks[0] == 'Defector':
count += 1
strategies.append(axelrod.TitForTat())
tournament = axelrod.Tournament(strategies)
results = tournament.play()
ranks = results.ranked_names
return count

strategies = [s() for s in axelrod.basic_strategies]
for s in strategies:
print(s)

strategies.append(axelrod.Defector())

tournament = axelrod.Tournament(strategies)

print(tournament.players)

results = tournament.play()

print(results.normalised_scores)

print(results.ranking)

print(results.ranked_names)

while results.ranked_names[0] == 'Defector':
strategies.append(axelrod.TitForTat()) # Adding a new tit for tat player
tournament = axelrod.Tournament(strategies)
results = tournament.play()
ranks = results.ranked_names

print(results.ranked_names.count('Tit For Tat'))

d = range(2, 50)
t = [find_number_of_tit_for_tat(n) for n in d]

print(max(t))


Reply to this email directly or view it on GitHub
#296 (comment)
.

@langner
Copy link
Member

langner commented Sep 15, 2015

would rather revert the basic strategies

That is also my vote, for all the reasons Vince pointed out. Everything else here is great.

@drvinceknight
Copy link
Member

Ok, so I woke up and realised I was going to be a pain to everyone... I think there might be merit in changing the basic_strategies as suggested by @marcharper. To move things along and because the other stuff in this PR needs to go in, I'm going to hit merge now (as @langner has ok'd) and open an issue to discuss :)

Great job @marcharper !

drvinceknight added a commit that referenced this pull request Sep 16, 2015
@drvinceknight drvinceknight merged commit 951269e into Axelrod-Python:master Sep 16, 2015
@drvinceknight
Copy link
Member

I have opened #297.

@marcharper marcharper deleted the strategy_reorg branch September 21, 2015 15:27
marcharper pushed a commit to marcharper/Axelrod that referenced this pull request Nov 2, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants