Add the K86R (MoreGrofman) Strategy #1124

buckbaskin · 2017-08-23T03:15:55Z

This strategy is implemented in response to issue #1093

I've implemented the MoreGrofman strategy as well as added tests to verify the functionality.

I'm opening a PR for early feedback and to check test coverage. I don't know if the pull request is ready to merge (probably not yet) and I'd appreciate feedback for how to improve the pull request.

drvinceknight

Thanks for this: looking great 👍

I've picked up a couple of minor things. I haven't looked through the logic but it is not implemented correctly as it's not matching up with the Fortran strategy.

Here is one such discrepancy:

>>> import axelrod as axl
>>> import axelrod_fortran as axlf
>>> moreG = axl.MoreGrofman()
>>> fortranG = axlf.Player('k86r')
>>> opponent = axl.AntiTitForTat()
>>> match = axl.Match((moreG, opponent), turns=15)
>>> match.play()
[(C, C),
 (C, D),
 (D, D),
 (D, C),
 (C, C),
 (C, D),
 (D, D),
 (D, C),
 (D, C),
 (D, C),
 (D, C),
 (D, C),
 (D, C),
 (C, C),
 (C, D)]
>>> fortran_match = axl.Match((fortranG, opponent), turns=15)
>>> fortran_match.play()
[(C, C),
 (C, D),
 (D, D),
 (D, C),
 (C, C),
 (C, D),
 (D, D),
 (D, C),
 (D, C),
 (D, C),
 (D, C),
 (D, C),
 (D, C),
 (D, C),
 (C, C)]

As this strategy is not stochastic the above (the 14th element is different) should be a good target for you to aim for.

Hopefully that's enough to find what might just be a small little error but if not we can help and identify what's wrong (perhaps our interpretation of the Fortran code is mistaken somewhere).

Note: the axelrod_fortran library (https://github.com/Axelrod-Python/axelrod-fortran) is a python interface to the actual fortran code. You don't need to install it but if you wanted to you'd need to compile the fortran code (https://github.com/Axelrod-Python/TourExec).

drvinceknight · 2017-08-23T06:33:09Z

axelrod/strategies/axelrod_second.py

+
+
+class MoreGrofman(Player):
+    '''


Can we change these to "

drvinceknight · 2017-08-23T06:35:36Z

axelrod/strategies/axelrod_second.py

+    1. First it cooperates on the first two rounds
+    2. For rounds 3-7 inclusive, it plays the same as the opponent's last move
+    3. Thereafter, it applies the following logic
+      - If its own previous move was C and the opponent has defected less than


CI is failing because this isn't building in the documentation. Need a space:

3. Thereafter, it applies the following logic - If its own previous move was C and the opponent has defected less than

drvinceknight · 2017-08-23T06:36:27Z

axelrod/strategies/axelrod_second.py

+            return opponent.history[-1]
+        # Logic for the rest of the game
+        else:
+            opponent_defections_last_7_rounds = 0


opponent_defections_last_7_rounds = opponent.history[-7:].count(D)

buckbaskin · 2017-08-24T03:21:36Z

@drvinceknight I believe I've updated the code to fix the areas you highlighted in your review comments. I also added some tests that use matches run with the Fortran library to verify that the Python implementation matches how the Fortran library behaves (up to 60 rounds vs. AntiTitForTat). I've pushed the changes to see if the docs-building is fixed, and I'd appreciate any additional feedback.

drvinceknight · 2017-08-24T08:57:15Z

I also added some tests that use matches run with the Fortran library to verify that the Python implementation matches how the Fortran library behaves (up to 60 rounds vs. AntiTitForTat).

Thanks @buckbaskin this is looking good. Before reviewing properly just to verify that we have a good implementation. I've run some of the new fingerprints implemented on #1125:

>>> import axelrod as axl
>>> import axelrod_fortran as axlf
>>> import numpy as np
>>> import matplotlib.pyplot as plt

>>> moreG = axl.MoreGrofman()
>>> fortranG = axlf.Player('k86r')

>>> moreGtf = axl.TransitiveFingerprint(moreG, number_opponents=20)
>>> fortranGtf = axl.TransitiveFingerprint(fortranG, number_opponents=20)

Against a spectrum of random opponents:

>>> axl.seed(0)
>>> _ = moreGtf.fingerprint(processes=0, repetitions=20000)  # Huge reps for stochasticity
>>> moreGtf.plot()

>>> axl.seed(0)
>>> _ = fortranGtf.fingerprint(processes=0, repetitions=20000)
>>> fortranGtf.plot();

These match up well.

>>> plt.imshow(np.abs(moreGtf.data - fortranGtf.data))
>>> plt.colorbar()

Against given deterministic strategies:

>>> basic = [s() for s in axl.basic_strategies]
>>> moreGtf_against_basic = axl.TransitiveFingerprint(moreG, opponents=basic)
>>> fortranGtf_against_basic = axl.TransitiveFingerprint(fortranG, opponents=basic)
>>> _ = moreGtf_against_basic.fingerprint(processes=0)
>>> moreGtf_against_basic.plot(display_names=True);

>>> _ = fortranGtf_against_basic.fingerprint(processes=0)
>>> fortranGtf_against_basic.plot(display_names=True);

Again a good match:

>>> np.array_equal(moreGtf_against_basic.data, fortranGtf_against_basic.data)
True

drvinceknight

So I see some discrepancies between the description and the code.

If these differences were there to ensure we have the same behaviour of the strategy (against AntiTitForTat for example), we should explain this in the docstring.

drvinceknight · 2017-08-24T18:55:12Z

axelrod/strategies/axelrod_second.py

+            return opponent.history[-1]
+        # Logic for the rest of the game
+        else:
+            opponent_defections_last_7_rounds = opponent.history[-8:-1].count(D) # (-7:) fails at 13, (-7:-1), (-8:), (-8:-1), (-6:), (-6:-1)


This doesn't seem correct: why doesn't opponent.history[-7:] work?

>>> ten_numbers = list(range(10)) >>> len(ten_numbers[-7:]) # 7 numbers 7 >>> len(ten_numbers[-7:-1]) # Only 6 numbers 6

I originally tried [-7:] but that resulted in what amounted to an off by one error for certain cases. This came to light when comparing the Python implementation with the Fortran implementation. I believe that the difference in indexes comes from the difference in Fortran indexing during do-loops/arrays (1 indexed and inclusive-inclusive) vs Python indexing into lists. The [-8:-1] indexing appears to match the behavior of the Fortran. I will remove the comment after because I was writing down potential off by one candidates to experiment with because I didn't really understand the Fortran indexing at the time.

Ok can you adjust the docstring description then because they're not what you are implemented:

If its own previous move was C and the opponent has defected less than twice in the last 7 rounds

I think we should say something like the mouthful that is the 7 rounds before the last one ... ?

drvinceknight · 2017-08-24T18:56:38Z

axelrod/strategies/axelrod_second.py

+        else:
+            opponent_defections_last_7_rounds = opponent.history[-8:-1].count(D) # (-7:) fails at 13, (-7:-1), (-8:), (-8:-1), (-6:), (-6:-1)
+            if self.history[-1] == C:
+                if opponent_defections_last_7_rounds <= 2:


The description says:

If its own previous move was C and the opponent has defected twice or more in the last 7 rounds, defect

So I believe this should be a strict inequality.

Based on the behavior in testing compared with the Fortran implementation and my attempt to understand the Fortran implementation, the logic is correct and I will adjust the comment. Is that sufficient?

drvinceknight · 2017-08-26T11:25:06Z

axelrod/tests/strategies/test_axelrod_second.py

+    name = "MoreGrofman"
+    player = axelrod.MoreGrofman
+    expected_classifier = {
+        'memory_depth': 7,


This now needs to be 8.

drvinceknight · 2017-08-26T11:42:58Z

I believe that the difference in indexes comes from the difference in Fortran indexing during do-loops/arrays (1 indexed and inclusive-inclusive) vs Python indexing into lists. The [-8:-1] indexing appears to match the behavior of the Fortran.

Having looked through the code I agree. Although I think it's actually due to the fact that the Fortran strategies all assume that prior to actually starting a match the opponent has cooperated (it's this "dummy" initial cooperation that I think is making the difference here).

Here is another example that shows that it's not in fact the last 7 moves but the 7 moves prior to the last one:

>>> import axelrod as axl
>>> import axelrod_fortran as axlf
>>> player = axlf.Player("k86r")
>>> sequence = "C" * 6 + "DDD"
>>> opponent = axl.Cycler(sequence)
>>> match = axl.Match((player, opponent), turns=11)
>>> match.play()
[(C, C),
 (C, C),
 (C, C),
 (C, C),
 (C, C),
 (C, C),
 (C, D),
 (C, D),
 (C, D),
 (C, C),
 (D, C)]

We see that the first 6 rounds are just pairs of cooperations.
The 7th round: the player cooperates
The 8th round: the player considers the first 6th moves of the opponent (keeping in mind that the Fortran strategies use an initial dummy cooperation move of the opponent I suspect this is the critical thing going on here) and there are no defections so it cooperates.
The 9th round: the player considers the first 7 moves: there is a single defection so it cooperates
The 10th round: the player considers moves from round 2 till 8: there are two defections so it cooperates
The 11th round: the player considers moves from round 3 till 9: there are 3 defections so it defects.

drvinceknight · 2017-08-26T12:12:18Z

axelrod/strategies/axelrod_second.py

+            if self.history[-1] == C:
+                if opponent_defections_last_8_rounds <= 2:
+                    return C
+                else:


no need to have an else, just replace the else with return D

Similarly below (line 264).

(Not a big deal: just a stylistic suggestion.)

drvinceknight

A part from getting the docs to build and other potential stylistic things that the other core maintainers might want I believe this is the correct implementation of the corresponding Fortran strategy.

drvinceknight · 2017-08-26T16:38:30Z

axelrod/strategies/axelrod_second.py

+    """
+    Submitted to Axelrod's second tournament by Bernard Grofman.
+
+    This strategy has 3 phases:


Also need a space here.

This strategy has 3 phases: 1. First

The sphinx make html documentation build works locally. Hopefully this will correct ongoing documentation CI build errors.

marcharper

Just some minor style comments, I'm assuming that strategy is correct as implemented.

marcharper · 2017-08-30T04:21:04Z

axelrod/strategies/axelrod_second.py

+        'manipulates_state': False
+    }
+
+    def __init__(self) -> None:


I don't think we need this __init__ if it simply calls the parent class __init__

marcharper · 2017-08-30T04:21:09Z

axelrod/strategies/axelrod_second.py

+
+class MoreGrofman(Player):
+    """
+    Submitted to Axelrod's second tournament by Bernard Grofman.


Can we add the Fortran rule name (K86R) to the docstring?

marcharper · 2017-08-30T04:21:43Z

axelrod/strategies/axelrod_second.py

+
+    def strategy(self, opponent: Player) -> Action:
+        # Cooperate on the first two moves
+        if not self.history or len(self.history) in [1]:


if len(self.history) < 2:

marcharper · 2017-08-30T04:27:08Z

axelrod/strategies/axelrod_second.py

+            #   in the last round and instead looks at the first 7 of the last
+            #   8 rounds.
+            opponent_defections_last_8_rounds = opponent.history[-8:-1].count(D)
+            if self.history[-1] == C:


Can this be simplified to:

if self.history[-1] == C and opponent_defections_last_8_rounds <= 2: return C if self.history[-1] == D and opponent_defections_last_8_rounds <= 1: return C return D

?

marcharper · 2017-08-30T04:27:52Z

axelrod/strategies/axelrod_second.py

+        # For rounds 3-7, play the opponent's last move
+        elif 2 <= len(self.history) <= 6:
+            return opponent.history[-1]
+        # Logic for the rest of the game


This comment isn't necessary

marcharper · 2017-08-30T04:28:56Z

axelrod/tests/strategies/test_axelrod_second.py

+        opponent = axelrod.AntiTitForTat()
+        # Actions come from a match run by Axelrod Fortran using Player('k86r')
+        actions = [
+            (C, C),


The spacing here is perhaps a little excessive, can we compress to just a couple of lines?

marcharper · 2017-08-30T04:29:05Z

axelrod/tests/strategies/test_axelrod_second.py

+        # Test to match the Fortran implementation for 30 rounds
+        opponent = axelrod.AntiTitForTat()
+        actions = [(C, C),
+            (C, D),


…grofman Resolve conflicts with the new Tranquilizer strategy.

drvinceknight · 2017-09-16T06:21:32Z

@marcharper I believe all your request have now been addressed and this is ready to go? :)

drvinceknight · 2017-09-19T20:10:49Z

Merging as all the stylistic requests have been addressed. Thanks @buckbaskin👍👍

buckbaskin added 5 commits August 22, 2017 21:32

Write out strategy logic

a93d74a

Fix some identified pep8 compliance issues

aca96ae

Update the strategies/_strategies.py file with MoreGrofman

1a9fa68

Add MoreGrofman strategy to the list of strategies

6ffdaea

Add tests to verify the MoreGrofman logic

bc50c84

buckbaskin mentioned this pull request Aug 23, 2017

K86R Strategy #1093

Closed

drvinceknight requested changes Aug 23, 2017

View reviewed changes

buckbaskin added 5 commits August 23, 2017 21:21

Make advised syntax changes with double quote, using count

900e885

Add test that demonstrates error in strategy logic

29de9ba

Add an extended test to match a 30 round Fortran match

f3625e8

Adjust indexing for an off-by-1 error. Test passes, will try longer test

e870730

Repair off-by-1 error in Python implementation and Add tests to verify

2480ca7

drvinceknight mentioned this pull request Aug 24, 2017

Implement transitive fingerprint #1125

Merged

drvinceknight requested changes Aug 24, 2017

View reviewed changes

buckbaskin added 2 commits August 26, 2017 06:20

Update comments to reflect behavior that matches Fortran behavior

79f0499

Clarify behavior re: inspection of history

8f64aad

drvinceknight requested changes Aug 26, 2017

View reviewed changes

buckbaskin added 2 commits August 26, 2017 09:41

Make a style change to else logic

37664eb

Change expected test memory depth to 8

1334535

drvinceknight requested changes Aug 26, 2017

View reviewed changes

Fix Spacing Issue that broke docs compilation

251da15

The sphinx make html documentation build works locally. Hopefully this will correct ongoing documentation CI build errors.

drvinceknight approved these changes Aug 29, 2017

View reviewed changes

drvinceknight added the ready-to-merge label Aug 29, 2017

drvinceknight mentioned this pull request Aug 29, 2017

Add MoreGrofman Axelrod-Python/axelrod-fortran#56

Closed

marcharper reviewed Aug 30, 2017

View reviewed changes

marcharper removed the ready-to-merge label Aug 30, 2017

buckbaskin added 3 commits September 5, 2017 19:22

Make style changes to MoreGrofman strategy

60ac912

Shorten the actions list for testing against Fortran examples

64efcae

Merge branch 'master' of github.com:Axelrod-Python/Axelrod into more-…

ad55ec5

…grofman Resolve conflicts with the new Tranquilizer strategy.

drvinceknight merged commit 7bd9c95 into Axelrod-Python:master Sep 19, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the K86R (MoreGrofman) Strategy #1124

Add the K86R (MoreGrofman) Strategy #1124

buckbaskin commented Aug 23, 2017

drvinceknight left a comment

drvinceknight Aug 23, 2017

drvinceknight Aug 23, 2017

drvinceknight Aug 23, 2017

buckbaskin commented Aug 24, 2017

drvinceknight commented Aug 24, 2017 •

edited

drvinceknight left a comment

drvinceknight Aug 24, 2017

buckbaskin Aug 26, 2017

drvinceknight Aug 26, 2017

drvinceknight Aug 24, 2017

buckbaskin Aug 26, 2017

drvinceknight Aug 26, 2017

drvinceknight commented Aug 26, 2017 •

edited

drvinceknight Aug 26, 2017 •

edited

drvinceknight left a comment

drvinceknight Aug 26, 2017

marcharper left a comment

marcharper Aug 30, 2017

marcharper Aug 30, 2017

marcharper Aug 30, 2017

marcharper Aug 30, 2017 •

edited

marcharper Aug 30, 2017

marcharper Aug 30, 2017

marcharper Aug 30, 2017

drvinceknight commented Sep 16, 2017

drvinceknight commented Sep 19, 2017

Add the K86R (MoreGrofman) Strategy #1124

Add the K86R (MoreGrofman) Strategy #1124

Conversation

buckbaskin commented Aug 23, 2017

drvinceknight left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

buckbaskin commented Aug 24, 2017

drvinceknight commented Aug 24, 2017 • edited

drvinceknight left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drvinceknight commented Aug 26, 2017 • edited

drvinceknight Aug 26, 2017 • edited

Choose a reason for hiding this comment

drvinceknight left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcharper left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcharper Aug 30, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drvinceknight commented Sep 16, 2017

drvinceknight commented Sep 19, 2017

drvinceknight commented Aug 24, 2017 •

edited

drvinceknight commented Aug 26, 2017 •

edited

drvinceknight Aug 26, 2017 •

edited

marcharper Aug 30, 2017 •

edited