-
Notifications
You must be signed in to change notification settings - Fork 264
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
Implemented Harrington (k75r from Axelrod's Second) #1146
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a big effort! As far as I can tell everything looks good.
axelrod/strategies/axelrod_second.py
Outdated
1. Check for defect and parity streaks. | ||
2. Check if cooperations are scheduled. | ||
3. Otherwise, | ||
- If turn < 37, Tit-for-Tat. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need a blank line here (this is what's failing on the CI):
3. Otherwise,
- If turn ...
axelrod/strategies/axelrod_second.py
Outdated
|
||
class Harrington(Player): | ||
""" | ||
Strategy submitted to Axelrod's second tournament by Paul Harringtn (K75R), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Harrington
axelrod/strategies/axelrod_second.py
Outdated
Strategy submitted to Axelrod's second tournament by Paul Harringtn (K75R), | ||
and came in eighth in that tournament. | ||
|
||
This strategy has three modes: Normal, and Fair-weather, Defect. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Normal, Fair-weather and Defect
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the source code https://github.com/Axelrod-Python/TourExec/blob/master/src/strategies/K75R.f, these names are of your choosing? Nothing wrong with that. Perhaps, we can add a sentence saying that though (just to avoid potential confusion):
..., and Defect. These mode names were not present in Harrington's submission.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we also add a sentence along the lines of what you wrote in the pdf: that the Fair-weather
mode occurs only in 1 very specific type of situation.
axelrod/strategies/axelrod_second.py
Outdated
|
||
if turn < 37: | ||
return self.try_return(opponent.history[-1], inc_parity=True) | ||
elif turn == 37: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can just be if turn == 37
axelrod/strategies/axelrod_second.py
Outdated
|
||
if turn == 38 and opponent.history[-1] == D and opponent.cooperations == 36: | ||
self.mode = "Fair-weather" | ||
# These flags would already be set from turn == 37 logic below. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can remove these two lines of inline comments These flags ...lower this turn.
axelrod/strategies/axelrod_second.py
Outdated
Strategy submitted to Axelrod's second tournament by Paul Harringtn (K75R), | ||
and came in eighth in that tournament. | ||
|
||
This strategy has three modes: Normal, and Fair-weather, Defect. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we also add a sentence along the lines of what you wrote in the pdf: that the Fair-weather
mode occurs only in 1 very specific type of situation.
# The defect streak will always be detected from here on, because it | ||
# doesn't reset. This logic comes before parity streaks or the turn- | ||
# based logic. | ||
self.versus_test(axelrod.Defector(), expected_actions=actions) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another thought, could we add a few attrs={"mode": "Normal", "recorded_defects": ...}
to the versus_test
call so that we specifically test some of these attribute values? Not necessarily overkill but at least the mode
and perhaps one or two others as you see fit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's cool. Didn't know that was possible.
This new error is confusing to me. Any idea? |
axelrod/strategies/axelrod_second.py
Outdated
else: | ||
self.exit_defect_meter -= 3 | ||
if self.exit_defect_meter >= 11: | ||
self.mode = "Normal" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coverage is failing because we don't have a test case that runs this code block:
axelrod/strategies/axelrod_second.py 436 4 99% 1196-1199
(That's the output of coveralls showing that lines 1196-1199 are not hit.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, interesting! I'll work on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this contribution! Since the code is complicated some more inline comments would be helpful.
@drvinceknight @meatballs I did not try to verify the logic in comparison to the Fortran source.
axelrod/strategies/axelrod_second.py
Outdated
In Normal and Fair-weather modes, the strategy begins by: | ||
|
||
- Update history | ||
- Detects random if turn is multiple of 15 and >=30. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
randomly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Detects random opponent? (Not overly fussed)
axelrod/strategies/axelrod_second.py
Outdated
parity streak that we're pointing to. If the parity streak that we're | ||
pointing to is then greater than `parity_limit` then reset the streak and | ||
cooperate immediately. `parity_limit` is initially set to five, but after | ||
its been hit eight times, it decreases to three. The parity streak that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it has
axelrod/strategies/axelrod_second.py
Outdated
pointing to is then greater than `parity_limit` then reset the streak and | ||
cooperate immediately. `parity_limit` is initially set to five, but after | ||
its been hit eight times, it decreases to three. The parity streak that | ||
we're pointing to also gets incremented if in normal mode and WE defect but |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we
axelrod/strategies/axelrod_second.py
Outdated
cooperate immediately. `parity_limit` is initially set to five, but after | ||
its been hit eight times, it decreases to three. The parity streak that | ||
we're pointing to also gets incremented if in normal mode and WE defect but | ||
not on turn 38, unless the result of a defect streak. Note that the parity |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unless we are defecting as a result of defect streak?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. I'll clarify.
axelrod/strategies/axelrod_second.py
Outdated
its been hit eight times, it decreases to three. The parity streak that | ||
we're pointing to also gets incremented if in normal mode and WE defect but | ||
not on turn 38, unless the result of a defect streak. Note that the parity | ||
streaks reset but the defect streak doesn't. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resets
axelrod/strategies/axelrod_second.py
Outdated
return False | ||
|
||
def detect_parity_streak(self, last_move): | ||
self.parity_bit = 1 - self.parity_bit # Flip bit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docstring please
axelrod/strategies/axelrod_second.py
Outdated
return False | ||
|
||
def detect_parity_streak(self, last_move): | ||
self.parity_bit = 1 - self.parity_bit # Flip bit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
two spaces before # (and several more below)
axelrod/strategies/axelrod_second.py
Outdated
if self.history[-1] == D: | ||
self.history_row += 2 | ||
|
||
# If generous 2 turn ago and opponent defected last turn |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 turns ago
axelrod/strategies/axelrod_second.py
Outdated
self.history_row += 2 | ||
|
||
# If generous 2 turn ago and opponent defected last turn | ||
if self.generous_n_turns_ago == 2 and opponent.history[-1] == D: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does "generous_n_turns_ago" mean "last generous n turns ago" or something else? It's ambiguous IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I can add "last_" here.
axelrod/strategies/axelrod_second.py
Outdated
if self.mode == "Fair-weather": | ||
if opponent.history[-1] == D: | ||
self.mode = "Normal" # Post-Defect is not possible | ||
#Continue below |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm trying to express here that this is the only place in the code where, following a mode-switch, we don't immediately return a value. Instead we actually treat this turn as a "Normal" mode code. I'll take your advice if you think it's really not worth mentioning. (Or if I should say more clearly.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't feel strongly about it but it's clear that the code continues to run IMO
@marcharper I'll make these changes and look for places where an in-line could be helpful. Thanks. |
Thanks! Some of the Fortran strategies are hard to understand so the extra inline comments are really helpful and will future-proof the code more completely. We don't want to repeat the current situation where the original strategies have very little documentation and we can't really know if they are implemented correctly. For example, there's a very rarely triggered behavior in this strategy which suggests that something wasn't implemented as intended (but we can't really know). |
I believe this is the one last thing that needs to be done review wise apart from the requested changes from @marcharper 👍 |
I think (hope) my attached PDF in original PR can be helpful here. |
yup it really did, to be clear I've gone through it myself and am happy, coupled with the agreement in the fingerprints I'm certain it's implemented correctly. Just in the case of this strategy which is pretty complex (kudos to what must have been a big translation effort @gaffney2010!) I think having a second set of eyes confirm would be good :) |
In this first commit just now, I modified the comments per Marc. In the second commit ("Cleaned up record history logic"), I very slightly modified the record history logic. [Because in the last version I was updating move_history after the player enters and leaves Defect mode, the same way that the Fortran strategy does (which gets complicated after leaving Defect mode). But I realized that since you can only enter Defect mode once, this matrix isn't being used again, so I just made it stop recording at that point, and was able to clean stuff up.] Because it was a logic change. I re-ran all the tests/FPs. It still looks good, and I can upload the FPs if needed. |
Sounds good, if you could re upload just so we have record here that'd be great 👍 |
axelrod/strategies/axelrod_second.py
Outdated
chi_squared += (expect - self.move_history[i, j]) ** 2 / expect | ||
|
||
# Caching value only for testing purposes, not used otherwise | ||
self.chi_squared = round(chi_squared, 3) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it doesn't look like you've gone with the suggestion of making the chi_squared
calculation it's own function which could be tested?
You could essentially keep this exactly as it is:
+ def calculate_chi_squared(self):
+ expected_matrix = np.outer(self.move_history.sum(axis=1),
+ self.move_history.sum(axis=0)) / denom
+
+ chi_squared = 0.0
+ for i in range(4):
+ for j in range(2):
+ expect = expected_matrix[i, j]
+ if expect > 1.0:
+ chi_squared += (expect - self.move_history[i, j]) ** 2 / expect
+
+ # Caching value only for testing purposes, not used otherwise
+ return chi_squared
Then in detect_random
you just use self.calculate_chi_squared
but then, for the test, you can directly run a match for your strategy and call the self.calculate_chi_squared
method to ensure it gives the correct value (with no need for the self.chi_squared
.
My other suggestion was to have this as a completely static method/function but it's also fine to leave it as a method of the class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't make the change, because it sounded like it was optional AND because I'm still not seeing how, under this alternative approach, I would test that move_history
is getting filled out right. If I understand your comment, it was that, this doesn't matter because I can test the Chi-Squared logic on my matrix by manually entering the matrix:
self.assertEqual(round(axelrod_second.calculate_chi_squared(np.matrix([[0, 2], [5, 6], [3, 6], [4, 2]])), 3), 2.395)
While I agree that this would successfully test the chi-squared logic. How do I test that move_history
correctly incremented to [[0, 2], [5, 6], [3, 6], [4, 2]] ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I see the connection between testing the move_history
and calculate_chi_squared
. As far as I can tell you're not testing that in the tests anyway:
+ actions += [(D, C)]
+ self.versus_test(axelrod.Random(0.5), expected_actions=actions, seed=10, attrs={"chi_squared": 2.395})
+ # The history matrix will be [[0, 2], [5, 6], [3, 6], [4, 2]]
You can keep this as a sub method of the class if you prefer in which case nothing here changes except you no longer need to artificially set self.chi_squared
as an attribute.
So that test (which if I understand correctly is the only purpose of this rounded chi_squared attribute which we'd rather not need) would be modified to be an actual match. Something like (I haven't run this):
axl.seed(10)
player = self.player()
match = axl.Match((player, axl.Random()), turns=len(expected_actions))
actions = match.play()
self.assertEqual(actions, expected_actions) # Just to be consistant with the current test.
self.assertEqual(round(player.calculate_chi_squared()), 2.395)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I see the connection between testing the move_history and calculate_chi_squared.
calculate_chi_squared uses move_history. You can get 2.935 precisely if move_history is filled out (correctly) as [[0, 2], [5, 6], [3, 6], [4, 2]]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I understand better now...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self.assertEqual(round(player.calculate_chi_squared()), 2.395)
I wasn't making the connection that the player would still be in the end-of-turn-30 state after you ran the axl.Match(), and I was trying to figure out how I could check everything in the Match() function, because I was imagining that the player would disappear afterwards. [I guess the way I wrote it, it would disappear.]
I'll change it like you have it. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
calculate_chi_squared uses move_history. You can get 2.935 precisely if move_history is filled out (correctly) as [[0, 2], [5, 6], [3, 6], [4, 2]]
Cool, so we can either:
-
create
compute_chi_squared
as it's own function (outside the class) that in the test gets passednp.array([[0, 2], [5, 6], [3, 6], [4, 2]])
,turn
as well asrecorded_defects
(which I believe it also uses). Socompute_chi_squared
would be a function that takes two arguments which is in turn called bydetect_random
. Something like:def detect_random(self, turn): chi_squared = compute_chi_squared(self.move_history, self.recorded_defects) if chi_squared > 3 ...
The test would be:
self.assertEqual(round(axelrod_second.calculate_chi_squared(np.matrix([[0, 2], [5, 6], [3, 6], [4, 2]])), ?, ?), 2.395) # Replace ? with the correct turn recorded_defects :)
-
Create a method which means we then need to test using a match.
I think option 1 is probably the cleanest way of doing this but 2 would do the trick also :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I understand better now...
Cool 👍
actions += [(D, D)] * 14 | ||
# Mutual defect for a while, then exit Defect mode with two coops | ||
actions += [(C, D)] * 2 | ||
self.versus_test(Rand_Then_Def, expected_actions=actions, seed=10, \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't need the slash here
'manipulates_state': False | ||
} | ||
|
||
def test_strategy(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for moving the chi-squared attribute. This test function is pretty long now, you might consider breaking it up into several test functions. (optional)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could also target specific functions with their own tests, e.g. for detect_streak
and friends.
actions = [(C, D), (D, C)] + [(C, C)] * 34 + [(D, C)] | ||
# Two cooperations scheduled after the 37-turn defection | ||
actions += [(C, C)] * 2 | ||
# TFT twice, then low-random # yields a DCC combo. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you want two lines for this comment?
# TFT twice, then low-random # yields a DCC combo. | ||
actions += [(C, C)] * 2 | ||
actions += [(D, C), (C, C), (C, C)] | ||
# Don't draw next random # until now. Again DCC. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
actions += [(D, C), (C, D), (D, C), (C, D), (C, C)] | ||
# This is the seventh time we've hit the limit. So do it once more. | ||
actions += [(C, D), (D, C), (C, D), (D, C), (C, D), (C, C)] | ||
# Now hit thi limit sooner |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thi --> this?
# The history matrix will be [[0, 2], [5, 6], [3, 6], [4, 2]] | ||
actions = match.play() | ||
self.assertEqual(actions, expected_actions) # Just to be consistant with the current test. | ||
self.assertEqual(round(player.calculate_chi_squared(len(expected_actions)),3), 2.395) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
space after comma (before 3); drop the round or use assertAlmostEqual
(which allows you to specify a number of places for equality):
self.assertAlmostEqual(a, b, places=4)
Link to the docs for assertAlmostEqual
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving; I've asked for some minor additional changes.
match = axelrod.Match((player, axelrod.Random()), turns=len(expected_actions)) | ||
# The history matrix will be [[0, 2], [5, 6], [3, 6], [4, 2]] | ||
actions = match.play() | ||
self.assertEqual(actions, expected_actions) # Just to be consistant with the current test. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove # Just to be consistant with current test
(that was just meant as a note to you in my comment but here it could be confusing).
Thanks for doing everything, this is looking great now, let's just remove that one inline comment which could be confusing later down the line :) Then this is good to go! 👍 |
Awesome: thank you for making all the requested tweaks. 👍 |
Below are the fingerprints for the basic strategies, the random, and against the six custom strategies that I used in the test file.
I've also attached a PDF that bridges the gap from the Fortran to what I have, along with some small amount of commentary on the code.
HarringtonNotes.pdf