-
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
884 grudger - with 1 to 3 issues #993
Conversation
only punishes after its opponent defects a specified amount of times consecutively. | ||
The punishment is in the form of a series of defections followed by a 'penance' of | ||
a series of consecutive cooperations. | ||
A generalization of the SoftGrudger strategy. SoftGrudger punishes by |
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.
changed docstring to fit line-llen = 80.
@@ -63,16 +63,16 @@ def __init__(self) -> None: | |||
def strategy(self, opponent: Player) -> Action: | |||
"""Begins by playing C, then plays D for mem_length rounds if the | |||
opponent ever plays D.""" | |||
if self.grudge_memory >= self.mem_length: |
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.
strategy docstring says defect for 10 rounds. strategy code said detect for 11 rounds. I changed the code to fit the docstring.
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.
👍
axelrod/strategies/grudger.py
Outdated
@@ -293,16 +294,18 @@ def strategy(self, opponent: Player) -> Action: | |||
The punishment is in the form of 'd' defections followed by a penance of | |||
'c' consecutive cooperations. | |||
""" |
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 startegy was changed to fit the edge cases where n=0 or d=0 or c=0. (see the tests at link for an example line 309 of tests). If the edge cases are invalid, then this change is totally unnecessary.
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.
If the edge cases are invalid,
I don't understand what you mean.
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.
sorry. that wasn't clear.
are n=0, d=0 and c=0 legal inputs?
if they are, the original code failed on n=0 and d=0. This change is to fix that.
if they are not allowed as inputs, then the original code works just fine and i can remove this change. (but perhaps it would be useful to add to n>=1 and d>=1
to the docstring)
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.
and on reflection, those cases would never be called. i will remove the changes.
|
||
# the default follows SoftGrudger | ||
super(TestGeneralSoftGrudger, self).test_strategy() | ||
|
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.
@drvinceknight - in the previous PR you asked me why i changed from the original test_strategy
to the following three tests. so I thought it over and here is my reasoning.
1 - I thought tests physically separated from each other and the default behavior tests were more clear.
2 - for n != 1
I wanted tests that showed the edges at and below the threshold. (and what happens with constant defections, which, for reasons unknown to me, I kept getting surprised about)
whether or not these are good reasons, I do think it useful to have this inherit from TestSoftGrudger
which will test the default behavior and reset
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 am a bit weary of PRs that change things that have been (very recently) added when the motivation for change seems stylistic. This takes a significant cognitive effort by the reviewers to make sure we're not breaking anything. If you recall there were a fair few times I had to explain why something was done a particular way in the fingerprint class.
Please keep these as they are. #884 is a simple issue: refactor tests to use versus_test
. If you catch a case that is missing by all means add it.
whether or not these are good reasons, I do think it useful to have this inherit from TestSoftGrudger which will test the default behavior and reset
test_reset
can actually be removed entirely by the way, this is now being tested by the parent class TestPlayer
.
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 apologize. i have great difficulty figuring these kinds of things out. this is an entirely new set of social rules far outside of my usual experience. the very explicit "this is annoying for this reason and causes this kind of trouble" is quite helpful. thank you for 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.
i'll wait on replies to the other two questions and revert to the original in the next PR.
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.
👍 Sorry for the late reply, I've been busy on other things. Looking through it now :)
actions = actions_start + subsequent * 5 | ||
self.versus_test(opponent, expected_actions=actions, | ||
init_kwargs=init_kwargs) | ||
|
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.
Are these edge cases valid or beyond the scope of what this class should be allowed to do?
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.
What do you meant by "valid"?
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.
are n=0, d=0 and c=0 legal/allowed inputs? or is it required that n>=1, d>=1, c>=1?
actions = actions_start + subsequent * 5 | ||
self.versus_test(opponent, expected_actions=actions, | ||
init_kwargs=init_kwargs) | ||
|
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 noticed in GoByMajority that it change self.name
to reflect the parameters but that this updates repr
to reflect the parameters. is one preferable to the other?
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.
Let's not change that for now.
@@ -63,16 +63,16 @@ def __init__(self) -> None: | |||
def strategy(self, opponent: Player) -> Action: | |||
"""Begins by playing C, then plays D for mem_length rounds if the | |||
opponent ever plays D.""" | |||
if self.grudge_memory >= self.mem_length: |
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.
👍
axelrod/strategies/grudger.py
Outdated
@@ -293,16 +294,18 @@ def strategy(self, opponent: Player) -> Action: | |||
The punishment is in the form of 'd' defections followed by a penance of | |||
'c' consecutive cooperations. | |||
""" |
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.
If the edge cases are invalid,
I don't understand what you mean.
actions = actions_start + subsequent * 5 | ||
self.versus_test(opponent, expected_actions=actions, | ||
init_kwargs=init_kwargs) | ||
|
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.
What do you meant by "valid"?
actions = actions_start + subsequent * 5 | ||
self.versus_test(opponent, expected_actions=actions, | ||
init_kwargs=init_kwargs) | ||
|
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.
Let's not change that for now.
|
||
# the default follows SoftGrudger | ||
super(TestGeneralSoftGrudger, self).test_strategy() | ||
|
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 am a bit weary of PRs that change things that have been (very recently) added when the motivation for change seems stylistic. This takes a significant cognitive effort by the reviewers to make sure we're not breaking anything. If you recall there were a fair few times I had to explain why something was done a particular way in the fingerprint class.
Please keep these as they are. #884 is a simple issue: refactor tests to use versus_test
. If you catch a case that is missing by all means add it.
whether or not these are good reasons, I do think it useful to have this inherit from TestSoftGrudger which will test the default behavior and reset
test_reset
can actually be removed entirely by the way, this is now being tested by the parent class TestPlayer
.
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.
1 tiny request.
# D ten times and then continue to check for defections. | ||
opponent = axl.Cooperator() | ||
actions = [(C, C)] * 20 | ||
self.versus_test(opponent, 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.
These look great but we're losing the attrs
test above. Could you add them in?
The function versus_test also accepts a dictionary parameter of attributes to check at the end of the match. For example this test checks if the player’s internal variable opponent_class is set to "Cooperative":
Let me know if I can assist.
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 great: thanks @eric-s-s 👍
grudger has one issue and possibly three.
GeneralSoftGrudger(n=1, d=10, c=0)
andGeneralSoftGrudger(n=1, d=4, c=2)