-
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
Add Neg strategy and change 4 method names for consistency #748
Conversation
Added Neg strategy Added Neg tests Updated _Strategies.py and all_strategies.rst to include Neg
Changed 4 instances of method name test_affect_of_strategy() to test_effect_of_strategy() for consistency
def strategy(self, opponent): | ||
# Random first move | ||
if(len(self.history) == 0): | ||
return random_choice(); |
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 would be better as:
if not self.history:
@@ -46,6 +46,7 @@ | |||
from .mindcontrol import MindController, MindWarper, MindBender | |||
from .mindreader import MindReader, ProtectedMindReader, MirrorMindReader | |||
from .mutual import Desperate, Hopeless, Willing | |||
from .neg import Neg | |||
from .oncebitten import OnceBitten, FoolMeOnce, ForgetfulFoolMeOnce, FoolMeForever |
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 strategy also needs to be added to the all_strategies list in this file
Thanks for the contribution @amkratz ! Just a couple of minor change requests from me. |
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.
A few minor changes from me. Let's change the strategy names (and class names) to use Negation.
|
||
Names: | ||
|
||
Neg - [No official reference found in docs, only found in 'Desired New Strategies' list: https://github.com/Axelrod-Python/Axelrod/issues/379] |
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.
Please list this as the reference: http://www.prisoners-dilemma.com/competition.html
Note that the full strategy name is Negation
, and we should use that as the class name.
Neg - [No official reference found in docs, only found in 'Desired New Strategies' list: https://github.com/Axelrod-Python/Axelrod/issues/379] | ||
""" | ||
|
||
name = "Neg" |
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.
Negation
|
||
name = "Neg" | ||
classifier = { | ||
'memory_depth': 1, # Four-Vector = (1.,0.,1.,0.) |
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 memory depth is correct but not the Four-Vector comment, so let's leave that off.
name = "Neg" | ||
classifier = { | ||
'memory_depth': 1, # Four-Vector = (1.,0.,1.,0.) | ||
'stochastic': False, |
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 is stochastic (first move is random).
Changed Neg to Negation Changed first move line and removed unnecessary comment Updated reference in Negation.py Updated list of all strategies in _strategies.py
Missing change from Neg to Negation in _strategies.py
Thank you for the help, I went ahead and made those changes listed. Sorry if this pull request is kind of messy, it's my first time submitting one. |
Doubler was added before and I didn't see it before I commited my updated files, so now I'm updating to include the doubler changes in hopes to correct the issue.
# Conflicts: # axelrod/strategies/_strategies.py # axelrod/strategies/doubler.py # axelrod/tests/unit/test_doubler.py # docs/reference/all_strategies.rst
Improper syntax was added to _strategies.py while I was trying to fix my repo. Hopefully this solves that issue.
Adjusted improper syntax in files that was added when trying to fix my repo
Not at all! It's quite good for a first PR on the project and we usually have at least a few minor comments with names and such. Thanks for the contribution! |
Looks like a few doc tests are failing now that we've marked the strategy as stochastic. You just need to manually edit the appropriate numbers in the documentation:
This will get the tests working on Travis. @meatballs @drvinceknight 👍 from me once the tests pass! |
Everything seems to be in order now, I appreciate the comments. I hope to tackle a few more new strategies soon! |
Thanks @amkratz !! |
Related to #379