-
Notifications
You must be signed in to change notification settings - Fork 268
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
N tit(s) for M tat(s) #1084
N tit(s) for M tat(s) #1084
Conversation
@Chadys looks like a doctest is failing:
|
Yes I saw, |
If I'm correct the default is TfT so I'd suggest not including this in |
I thought all strategies had to be in |
Not quite :) This is similar to I'll review this properly tomorrow, logging off for the evening. |
…emoval of SlowTitForTwoTats
Can we add one instance to the library as a strategy, with say N=3 M=2 or similar? |
Sounds good to me. Perhaps that could be the default?
…On Tue, 25 Jul 2017, 01:49 Marc Harper, ***@***.***> wrote:
Can we add one instance to the library as a strategy, with say N=3 M=2 or
similar?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#1084 (comment)>,
or mute the thread
</~https://github.com/notifications/unsubscribe-auth/ACCGWrVGvIuib2lcb9EcxlsNWg8c_465ks5sRTuvgaJpZM4OhPd_>
.
|
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 very minor comments from me @Chadys. This looks excellent and good to go otherwise.
Just the other comment from @marcharper about including a default. I see two ways to go:
- Make 3, 2 the default;
- Include a single class 3TitsFor2Tats that's inherited. There are examples of this in the memory_one strategies where we have the generic memory one and LR player as well as a number of defaults.
I don't have a strong feeling either way on this :)
axelrod/strategies/titfortat.py
Outdated
""" | ||
Parameters | ||
---------- | ||
N, int |
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.
Because of the type hints let's drop the type in the docstrings (in case at some point they are changed and we forget to change them in the docstrings).
axelrod/strategies/titfortat.py
Outdated
if not self.M or opponent.history[-self.M:].count(D) == self.M: | ||
self.retaliate_count = self.N | ||
if self.retaliate_count: | ||
self.retaliate_count-=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.
PEP8: self.retaliate_count -= 1
All done, I went with the change in default values solution ! |
Looks good to me 👍 |
Resolve #1073 and add the NTitsForMTats asked for in #379