Skip to content
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.

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

Implement SpitefulTitForTat strategy from PRISON #749

Merged
merged 9 commits into from
Oct 28, 2016

Conversation

WonkySpecs
Copy link
Contributor

From the list #379

Copy link
Member

@drvinceknight drvinceknight left a comment

Choose a reason for hiding this comment

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

Thanks for the awesome contribution @WonkySpecs.

My requests are mainly just asking for a couple more tests.


Names:

- Spiteful Tit For Tat
Copy link
Member

Choose a reason for hiding this comment

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

Could you add:

- Spiteful Tit For Tat: [PRISON1998]_


name = 'Spiteful Tit For Tat'
classifier = {
'memory_depth': 2,
Copy link
Member

Choose a reason for hiding this comment

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

The memory_depth is float('inf') because of the eternal defection that can kick in.

if len(self.history) == 0:
return C

if opponent.history[-2:]==[D,D]:
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 have space either side of ==, so:

if opponent.history[-2:] == [D, D]:

(PEP8)

name = "Spiteful Tit For Tat"
player = axelrod.SpitefulTitForTat
expected_classifier = {
'memory_depth': 2,
Copy link
Member

Choose a reason for hiding this comment

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

Memory depth will need to be adjusted.

self.markov_test([C, D, C, D])
self.responses_test([C] * 4, [C, C, C, C], [C])
self.responses_test([C] * 5, [C, C, C, C, D], [D])
self.responses_test([C] * 5, [C, C, D, D, C], [D])
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 add a test for the self.retaliating attribute directly and also a test that checks that when resetting the strategy this is reset as expected.

You can do my first request using the responses_test, which you can see here http://axelrod.readthedocs.io/en/latest/tutorials/contributing/strategy/writing_test_for_the_new_strategy.html:

The function responses_test also accepts a dictionary parameter of attributes to check at the end of the checks. For example this test checks if the player’s internal variable opponent_class is set to "Cooperative":

So here you would have:

        self.responses_test([C] * 4, [C, C, C, C], [C], attrs={"retaliating": False})
        self.responses_test([C] * 5, [C, C, C, C, D], [D], attrs={"retaliating": False})
        self.responses_test([C] * 5, [C, C, D, D, C], [D], attrs={"retaliating": True}) 

Copy link
Member

Choose a reason for hiding this comment

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

To test the reset method you can add another method (with a different name than test_reset):

    def test_reset_retaliating(self):
        player = self.player()
        player.retaliating = True
        player.reset()
        self.assertFalse(player.retaliating)

Copy link
Member

Choose a reason for hiding this comment

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

Also, could we have the blank line at the end of the file put back (PEP8)?


def strategy(self, opponent):
# First move
if len(self.history) == 0:
Copy link
Member

Choose a reason for hiding this comment

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

would be better as:

if not self.history:


def __init__(self):
Player.__init__(self)
self.retaliating=False
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 have some space around the '=' (PEP8)?

return C

if opponent.history[-2:]==[D,D]:
self.retaliating=True
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 have some space around the '=' (PEP8)?


def reset(self):
Player.reset(self)
self.retaliating=False
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 have some space around the '=' (PEP8)?

Copy link
Member

Choose a reason for hiding this comment

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

Also, could we have the blank line at the end of the file put back (PEP8)?

self.markov_test([C, D, C, D])
self.responses_test([C] * 4, [C, C, C, C], [C])
self.responses_test([C] * 5, [C, C, C, C, D], [D])
self.responses_test([C] * 5, [C, C, D, D, C], [D])
Copy link
Member

Choose a reason for hiding this comment

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

Also, could we have the blank line at the end of the file put back (PEP8)?

@meatballs
Copy link
Member

Thanks also from me @WonkySpecs My comments are just PEP8 style issues.

@@ -34,7 +34,7 @@ class TitForTat(Player):
def strategy(self, opponent):
"""This is the actual strategy"""
# First move
if len(self.history) == 0:
if not self.history:
Copy link
Member

Choose a reason for hiding this comment

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

👍 Thanks for spotting that one!

@@ -295,7 +295,7 @@ def __init__(self, deadlock_threshold=3, randomness_threshold=8):

def strategy(self, opponent):
# Cooperate on the first move
if len(self.history) == 0:
if not self.history:
Copy link
Member

Choose a reason for hiding this comment

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

👍 Again

@WonkySpecs
Copy link
Contributor Author

Because of the change to memory_depth, i need to change classification_of_strategies back, Is it better to do this as a new commit or revert the other one?

@drvinceknight
Copy link
Member

Up to you :) Reverting a commit create a new commit anyway, but do whatever you feel is easier.

@@ -554,3 +554,50 @@ def reset(self):
def __repr__(self):

return "%s: %s" % (self.name, round(self.rate, 2))

Copy link
Member

Choose a reason for hiding this comment

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

PEP8 - Should be two blank lines before a class definition

if not self.history:
return C

if opponent.history[-2:] == [D,D]:
Copy link
Member

Choose a reason for hiding this comment

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

PEP8:
if opponent.history[-2:] == [D, D]:

return C

if opponent.history[-2:] == [D,D]:
self.retaliating=True
Copy link
Member

Choose a reason for hiding this comment

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

PEP8:
self.retaliating = True

Player.reset(self)
self.retaliating = False


Copy link
Member

Choose a reason for hiding this comment

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

PEP8:

Should be one blank line at the end of file, not two.

@@ -497,4 +497,32 @@ def test_world_rate_reset(self):
self.assertEqual(p2.rate, 0.5)


Copy link
Member

Choose a reason for hiding this comment

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

There are whitespace characters in these two lines that shouldn't be there.

self.markov_test([C, D, C, D])
self.responses_test([C] * 4, [C, C, C, C], [C], attrs = {"retaliating": False})
self.responses_test([C] * 5, [C, C, C, C, D], [D], attrs = {"retaliating": False})
self.responses_test([C] * 5, [C, C, D, D, C], [D], attrs = {"retaliating": True})
Copy link
Member

Choose a reason for hiding this comment

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

PEP8

The three lines above have spaces around the '=' which shouldn't be there.

player = self.player()
player.retaliating = True
player.reset()
self.assertFalse(player.retaliating)

Copy link
Member

Choose a reason for hiding this comment

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

There is an extra blank line at the end of the file which shouldn't be there

@meatballs
Copy link
Member

@WonkySpecs I know I'm pointing out small PEP8 errors which already exist elsewhere in the library, but we are making an effort to improve!

@meatballs
Copy link
Member

Having now seen how many PEP8 errors there are elsewhere within titfortat.py and test_titfortat.py, I think I may put in a PR to clean them up a little.

@WonkySpecs
Copy link
Contributor Author

Fully understand wanting to keep everything clean, think I've made all the requested changes and thanks for all the really detailed feedback, its been very helpful :)

@drvinceknight
Copy link
Member

Having now seen how many PEP8 errors there are elsewhere within titfortat.py and test_titfortat.py, I think I may put in a PR to clean them up a little.

Good idea @meatballs. :)

@meatballs meatballs merged commit 0d8f3ca into Axelrod-Python:master Oct 28, 2016
@meatballs
Copy link
Member

Many thanks @WonkySpecs

I've sent you an invitation to join the team. If you want the logo to appear on your profile, visit https://github.com/orgs/Axelrod-Python/people and set your membership to 'public'

You might also want to join our chat room: https://gitter.im/Axelrod-Python/Axelrod You'll normally find at least one of us lurking in there!

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.

None yet

3 participants