-
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 gobymajority, gradualkiller, grudger, grumpy #989
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.
this is simply fitting docstring to 80 chars per line.
@@ -67,12 +67,12 @@ def strategy(self, opponent: Player) -> Action: | |||
self.grudge_memory = 0 | |||
self.grudged = False | |||
|
|||
if not self.grudged and D in opponent.history[-1:]: |
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.
The code and docstring didn't match. the docstring said to play D for 10 rounds, but the code played D for 11 rounds. I wasn't sure which to follow, so for now, I changed the code to fit the docstring.
for a small issue, is this a correct way to raise 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.
The code and docstring didn't match. the docstring said to play D for 10 rounds, but the code played D for 11 rounds. I wasn't sure which to follow, so for now, I changed the code to fit the docstring.
for a small issue, is this a correct way to raise it?
Yup: this is fine and I think you've changed it correctly. The behaviour was not matching the description. 👍
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 feel these changes have made the tests a bit more obscure. If possible I'd prefer if we stuck to the very explicit:
opponent = ...
actions = ...
self.versus_test
If some of the setup for opponent
and actions
need to be repeated I would prefer that in favour of very explicit tests. Of course this is not a hard fast rule and in some cases there might just not be a way around things but looking through this I don't think the helper functions are helping. (Although without studying them closely I don't actually understand some of them which is my point)
[D] * (L // 2 + 1) + [C] * (L // 2 - 1)) | ||
opponent_actions = [C] * memory_depth + [D] * memory_depth | ||
""" | ||
with memory_depth=3 always switches after |
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 these docstrings be made in to inline comments #
(or actual docstrings).
self.responses_test([D], [C, C, D, D, D, D, C, C, D], | ||
[C, D, C, C, C, C, C, D, C]) | ||
vs_cooperator = [(C, C)] * 30 | ||
self.versus_test(axl.Cooperator(), expected_actions=vs_cooperator) |
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.
To be in line with tests already written could we stick with actions=[(C, C)] * 30
instead of vs_cooperator
. (Similarly elsewhere).
init_kwargs={"n": 2}) | ||
|
||
# Testing n=1, d=1, c=1 | ||
actions = [(C, D), (D, D), (C, C), (C, C), (C, D), (D, D), (C, C), (C, C)] |
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.
Why are you re writing these tests? They are already in the correct form.
self.versus_test(axl.MockPlayer(actions=opponent), | ||
expected_actions=expected) | ||
|
||
def get_actions_for_test(self, opponent_start, action_six_and_seven, |
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 they're truly necessary it's nicer to not make use of these helper functions and write out tests explicitly: otherwise technically we also want a test for this helper function.
If we do actually need this helper function then it should certainly be better documented.
|
||
def test_set_soft_to_false(self): | ||
self.eq_play = D | ||
expected, opponent_actions = self.get_infinite_memory_depth_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.
Not a fan of these helper functions. I have no idea what this does and subsequently am not sure what this test is doing. Can you just write out the test explicitly?
I'd prefer it if we kept the tests very clear (technically these helper functions need tests). Of course there might be a valid argument for using a helper function here but as is it just obfuscates the test here (no docstring etc). In this case for example it looks like you're just concatenating 3 lists together: so I'd just go with that.
expected, opponent_actions = self.get_infinite_memory_depth_actions() | ||
self.versus_test(MockPlayer(actions=opponent_actions), | ||
expected_actions=expected, init_kwargs={'soft': False}) | ||
self.eq_play = C |
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.
Not sure why this needs to be a class attribute. (I'm not actually sure what this is doing.)
If it's helpful: please do feel free to open multiple PRs for each module (it helps with reviewing especially when/if you're going to make more than just the straightforward changes converting |
yes. that seems like a good idea. i'll open PR's for each one. close this one? (i'll, of course be working off the comments you made and change those things before pushing up the new PR's. this will be in a couple of days) |
bah! wrong button. i meant to ask if i should close, not close. >( |
Sounds good to me :) 👍 |
#884
There is a likely off-by-one error in ForgetfulGrudger. The docstring was the following.
mem_length was 10 but played D for 11 rounds. I changed the code to follow the docstring.