-
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 new strategy SelfSteem #866
Conversation
One doubt I have is to whether to check for cases where the strategy returns a random |
axelrod/strategies/selfsteem.py
Outdated
If f > 0.95, 'ego' of the algorithm is inflated; always defects. | ||
If 0.95 > |f| > 0.3, rational behavior; follows TitForTat algortithm. | ||
If 0.3 > f > -0.3; random behavior. | ||
If f < -0.95, algorithm is at rock bottom; always cooperates. |
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 you add Names ...
see: http://axelrod.readthedocs.io/en/latest/tutorials/contributing/strategy/writing_the_new_strategy.html This will also require you to add the reference to docs/reference/bibliography.rst
.
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 you also add this strategy to docs/reference/all_strategies.rst
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/selfsteem.py
Outdated
|
||
def strategy(self, opponent: Player) -> Action: | ||
turns_number = len(self.history) | ||
sine_value = sin(2*pi*turns_number/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.
PEP8: 2 * pi * ...
self.responses_test([D], [C] * 16, [D] * 16) | ||
self.responses_test([C], [D] * 9, [C] * 9) | ||
|
||
|
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 you get rid of the blank lines but also include a test for the random behaviour, you can do this by passing a seed=
to the responses_test
function: http://axelrod.readthedocs.io/en/latest/tutorials/contributing/strategy/writing_test_for_the_new_strategy.html
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 exactly is happening here with seed
?
I saw in seed =1
and seed = 2
being passed. What does it 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.
So a random process starts with a random seed. This just ensures the "randomness" can be repeated:
>>> import random
>>> random.seed(0)
>>> random.random()
0.8444218515250481
>>> random.seed(0)
0.8444218515250481
The seed=1
and seed=2
just set those seeds for the strategies.
@drvinceknight Does the build error have something to do with my PR? |
docs/reference/all_strategies.rst
Outdated
@@ -135,6 +135,9 @@ Here are the docstrings of all the strategies in the library. | |||
.. automodule:: axelrod.strategies.shortmem | |||
:members: | |||
:undoc-members: | |||
.. automodule:: axelrod.strategies.selfsteem | |||
:members: |
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.
Your indentation levels aren't correct here. Compare them to the other strategies.
The error is in the building of the docs:
It might be due to the indentation I pointed out but it could be something else. If you run |
There are other errors, I'm taking a look now. |
axelrod/strategies/selfsteem.py
Outdated
with the current iteration. | ||
|
||
If f > 0.95, 'ego' of the algorithm is inflated; always defects. | ||
If 0.95 > |f| > 0.3, rational behavior; follows TitForTat algortithm. |
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 line is creating an error as sphinx is confused by |f|
. I suggest replacing it with abs(f)
.
axelrod/strategies/selfsteem.py
Outdated
return opponent.history[-1] | ||
|
||
elif sine_value < 0.3 and sine_value > -0.3: | ||
return random.choice((C, 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.
Better to use the inbuilt random_choice()
from axelrod.random_
.
axelrod/strategies/selfsteem.py
Outdated
- SelfSteem: [Andre2013]_ | ||
""" | ||
|
||
name = 'SelfSteem' |
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 is the name of this strategy?
SelfSteem
or SelfEsteem
? Across module names etc you seem to be combining these.
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 SelfSteem
. I mistakenly named the test test_selfesteem
instead of test_selfsteem
.
My suggestions there fix some of the errors but not all of them. My hunch is that it's something offsetting the random seed somewhere but I'd need more time to look through it. I'll try and get to it over the weekend if you don't find the fix but I'm travelling so it might be delayed :) I'm sure we'll figure it out :) |
self.responses_test([D], [C] * 16, [D] * 16) | ||
self.responses_test([C], [D] * 9, [C] * 9) | ||
|
||
|
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 a random process starts with a random seed. This just ensures the "randomness" can be repeated:
>>> import random
>>> random.seed(0)
>>> random.random()
0.8444218515250481
>>> random.seed(0)
0.8444218515250481
The seed=1
and seed=2
just set those seeds for the strategies.
Fix the classification and also add a test for stochastic behaviour.
@janga1997 I've opened a PR on your fork: janga1997#1 If you merge that in, it will automatically update the PR with my suggested commits. The main error you were getting was caused because of a mis classification: you were saying the strategy was not stochastic and so it was being used in other tests (but it is stochastic). |
Fix strategy.
@drvinceknight I've been seeing this error since the beginning.
|
@drvinceknight And why did we merge #839 ? |
Yup, that will be the doctest for this page: http://axelrod.readthedocs.io/en/latest/tutorials/advanced/classification_of_strategies.html Just bump the number. |
You're completely correct! I'm not sure how that got by. I'm looking at it now. I might revert it. |
@drvinceknight I guess that was the main error. The tests seem to be passing now. |
Yup, the only thing left is those errors about the type annotations in the moran process and as I've written on #868 that's not being picked up by travis :) The strategy looks good 👍 I noted there's one point in the paper:
As far as I can tell there's no indication as to how that is done right? Could you also tweak in |
@drvinceknight It didn't specify anything, not even how much the phase change is supposed to be, and in which direction. |
Yeah I see that. Not sure if this means it's worth rewording the docstring to make a note of this. Something like "This strategy is based on ..., however due to a limited number of details in the original description the sin function is not shifted as in the original work." Any thoughts @marcharper? I'm happy with everything else. |
I agree with @drvinceknight -- let's make a note in a docstring and email the authors for a clarification. Unfortunately this is a common issue and one of the goals of the library is to enable reproducible results going forward. It's clear that journals are not requiring source code disclosures and peer reviewers are not demanding sufficient descriptions :/ |
|
||
class SelfSteem(Player): | ||
""" | ||
This strategy is based on the feeling with the same name. |
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 esteem?
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 means self esteem, that's how its named in the paper. I decided not to change it.
""" | ||
This strategy is based on the feeling with the same name. | ||
It is modeled on the sine curve(f = sin( 2* pi * n / 10 )), which varies | ||
with the current iteration. |
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.
current iteration n
is the round number? (I see below the answer is yes but the docstring could say so.)
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.
|
||
def strategy(self, opponent: Player) -> Action: | ||
turns_number = len(self.history) | ||
sine_value = sin(2 * pi * turns_number / 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.
Just checking -- is this integer or float division in the reference?
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.
In python3, doesn't it return a float by default?
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 does, I'm just making sure that's correct.
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, we do need a float division.
@marcharper I've emailed the authors of the paper regarding what I'm trying to do here, and what information I require. Yet to receive a reply. |
Great, let's give them a day or two to reply. If we don't hear anything then we'll go with our best guess. |
#379
I've tried to make sure everything was done, but I might have left something. Do take a look.
To see the original paper, Page 3