-
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
Automatically set strategy classes (and transformers) __repr__ to include parameters. #802 #953
Changes from 20 commits
5dfd22b
8ebd43f
b189a93
08477d9
239492e
86ed15d
c150911
bf83ba6
8296a89
a40ecc0
6d2dfb4
6bd8b04
eff83ec
0c9519b
f5f76be
a10e2f0
e14dd08
d70a2ac
b7b8ac5
e744914
fa898a4
180e28c
f603acb
c06dca4
66ac41f
1151f2c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,7 @@ | |
from functools import wraps | ||
import random | ||
import copy | ||
import inspect | ||
|
||
from axelrod.actions import Actions, flip_action, Action | ||
from .game import DefaultGame | ||
|
@@ -73,7 +74,7 @@ class Player(object): | |
""" | ||
|
||
name = "Player" | ||
classifier = {} # type: Dict[str, Any] | ||
classifier = {} # type: Dict[str, Any] | ||
default_classifier = { | ||
'stochastic': False, | ||
'memory_depth': float('inf'), | ||
|
@@ -87,10 +88,27 @@ class Player(object): | |
def __new__(cls, *args, **kwargs): | ||
"""Caches arguments for Player cloning.""" | ||
obj = super().__new__(cls) | ||
obj.init_args = args | ||
obj.init_kwargs = kwargs | ||
obj.init_kwargs = cls.init_params(*args, **kwargs) | ||
return obj | ||
|
||
@classmethod | ||
def init_params(cls, *args, **kwargs): | ||
""" | ||
Return a dictionary containing the init parameters of a strategy (without 'self'). | ||
Use *args and *kwargs as value if specified | ||
and complete the rest with the default values. | ||
""" | ||
sig = inspect.signature(cls.__init__) | ||
# the 'self' parameter needs to be removed or the first *args will be assigned to it | ||
self_param = sig.parameters.get('self', None) | ||
if self_param is not None: | ||
new_params = list(sig.parameters.values()) | ||
new_params.remove(self_param) | ||
sig = sig.replace(parameters=new_params) | ||
boundargs = sig.bind_partial(*args, **kwargs) | ||
boundargs.apply_defaults() | ||
return boundargs.arguments | ||
|
||
def __init__(self): | ||
"""Initiates an empty history and 0 score for a player.""" | ||
self.history = [] | ||
|
@@ -122,8 +140,15 @@ def set_match_attributes(self, length=-1, game=None, noise=0): | |
self.receive_match_attributes() | ||
|
||
def __repr__(self): | ||
"""The string method for the strategy.""" | ||
return self.name | ||
"""The string method for the strategy. | ||
Appends the `__init__` parameters to the strategy's name.""" | ||
name = self.name | ||
prefix = ': ' | ||
gen = (value for value in self.init_kwargs.values() if value is not None) | ||
for value in gen: | ||
name = ''.join([name, prefix, str(value if not isinstance(value, float) else round(value, 2))]) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Technically strategies won't have unique names since There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would prefer it if we didn't round. Does that make things messy in some cases? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Originally, the
So maybe use a bigger value for round or, if you prefer to be really precise and you don't mind There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suppose ultimately the floats in the
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I prefer less aesthetically pleasing in favor of more uniqueness in this case There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
prefix = ', ' | ||
return name | ||
|
||
@staticmethod | ||
def _add_noise(noise, s1, s2): | ||
|
@@ -158,7 +183,7 @@ def clone(self): | |
# be significant changes required throughout the library. | ||
# Override in special cases only if absolutely necessary | ||
cls = self.__class__ | ||
new_player = cls(*self.init_args, **self.init_kwargs) | ||
new_player = cls(**self.init_kwargs) | ||
new_player.match_attributes = copy.copy(self.match_attributes) | ||
return new_player | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -51,14 +51,16 @@ def __init__(self, memory_depth: int = float('inf'), soft: bool = True) -> None: | |
self.memory = self.classifier['memory_depth'] | ||
else: | ||
self.memory = 0 | ||
|
||
self.name = ( | ||
'Go By Majority' + (self.memory > 0) * (": %i" % self.memory)) | ||
if self.soft: | ||
self.name = "Soft " + self.name | ||
else: | ||
self.name = "Hard " + self.name | ||
|
||
def __repr__(self): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the default There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When I don't override
The 'True' is redundant with the 'Soft' name, so I guess I should effectively get rid of this override but also get rid of this lines (gobymajority.py L54-59) : self.name = (
'Go By Majority' + (self.memory > 0) * (": %i" % self.memory))
if self.soft:
self.name = "Soft " + self.name
else:
self.name = "Hard " + self.name and add the 'Soft' manually in the names of the GoByMajority_number ? But that means the GoByMajority main class will no longer be call 'Soft Go By Majority' or 'Hard Go By Majority' but 'Go By Majority: True' or 'Go By Majority: False', are we okay with that ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I'm fine with that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should prefer "Soft" since it's the name in literature There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Knowing that it would just apply for the main GoByMajority, the SoftGoByMajority40 would still be called 'Soft Go By Majority 40', the same for all the other versions, and it would avoid an override of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For now I did the 'Go By Majority: True' option but if everyone agree on the other one, I will put back the override There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On reflection I agree with @marcharper, keeping in line with the literature is good. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok ! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do I still keep the ': inf' though ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps it's not needed for the base one. |
||
return self.name | ||
|
||
def strategy(self, opponent: Player) -> Action: | ||
"""This is affected by the history of the 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.
Does this conditional need to be present?
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 should always be a 'self' parameters to get rid of, but since I'm calling
list.remove
which would fail if I call it with None, better be safe than sorry I guess. But I agree that there is no reason an__init__
would ever have no 'self' parameter so I guess I could remove this test.