-
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
Adds Dual Transformer and Joss-Ann Transformer #746 #758
Conversation
def dual_strategy(self, opponent): | ||
from copy import deepcopy | ||
fresh_opponent = deepcopy(opponent) | ||
self.original.play(fresh_opponent) |
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.
should this be self.original_player.play
?
That appears to be the name of the attribute which is set at line 187 in strategy_transformers.py
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 believe this method is not actually necessary (the transformer does not use it). Can it just be removed?
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.
(There's no need to modify the player class at all, it can all just be done as a transformer.)
def noisy_wrapper(player, opponent, action, noise=0.05): | ||
"""Applies flip_action at the class level.""" | ||
r = random.random() | ||
if r < noise: | ||
return flip_action(action) | ||
return action | ||
|
||
|
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.
👍 for fixing PEP8 errors!
MixedTransformer = StrategyTransformerFactory( | ||
mixed_wrapper, name_prefix="Mutated") | ||
|
||
|
||
def joss_ann_wrapper(player, opponent, proposed_action, probability): | ||
"""Wraps the players strategy function to produce the Joss-Ann. |
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 is it called the Joss-Ann? Is that a reference to some paper somewhere? If so, can we have a link in here?
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 we include the bibliographic reference too as well (so with [Ashlock...]_
and the relevant info in the bibliography.rst file.
return action | ||
|
||
|
||
JossAnnTransformer = StrategyTransformerFactory(joss_ann_wrapper, |
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 we have both parameters on one new line to match the other transformer definitions?
|
||
The Dual of a strategy will return the exact opposite set of moves to the | ||
original strategy when both are faced with the same history. | ||
|
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 needs a reference to Ashlock's paper so we can see where it comes from.
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.
👍
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.
Various things.
Could you add a title to this PR and some context for any reviewer who might not be familiar.
Also: I suggest you add tests before you refactor, it'll make things easier to review but it will also make the refactoring simpler.
MixedTransformer = StrategyTransformerFactory( | ||
mixed_wrapper, name_prefix="Mutated") | ||
|
||
|
||
def joss_ann_wrapper(player, opponent, proposed_action, probability): | ||
"""Wraps the players strategy function to produce the Joss-Ann. |
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 we include the bibliographic reference too as well (so with [Ashlock...]_
and the relevant info in the bibliography.rst file.
probability += (remaining_probability,) | ||
options = [C, D, proposed_action] | ||
action = choice(options, p=probability) | ||
return action |
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 we not implement this as a particular case of the Mixed Strategy transformer (by mixing between the probe and Cooperator and Defector)? This would reduce duplication.
def dual_strategy(self, opponent): | ||
from copy import deepcopy | ||
fresh_opponent = deepcopy(opponent) | ||
self.original.play(fresh_opponent) |
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 believe this method is not actually necessary (the transformer does not use it). Can it just be removed?
|
||
The Dual of a strategy will return the exact opposite set of moves to the | ||
original strategy when both are faced with the same history. | ||
|
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 should I put a link to the relevant paper (in both the Dual and JossAnn Transformers) and then also add something to |
Yes please. |
(And in the docs you'll be able to point to the reference in the bibliography. Sphinx will render it nicely.) |
Adds Dual Transformer and Joss-Ann Transformer. These are both required when creating the fingerprint of a strategy as described in http://doi.org/10.1109/ITW.2010.5593352
These are no tests for these yet, but I thought I'd open a pull request to make sure people are happy with the implementation.
Edit - there are now tests.