-
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
Conversation
It looks like tests are failing because of ordering of the dictionaries being different based on Python versions? (I've looked at this very briefly: https://travis-ci.org/Axelrod-Python/Axelrod/jobs/217220481). Perhaps you can sort the keys? |
Yes I'm mostly sure I know where the problem is, I'll get to it. |
I made a list instead of a dict, so order should now be ok for both python 3.5 and 3.6. Also, for consistency sake, I should make |
Ok so now I have another problem on python 3.5 :
For those :
https://travis-ci.org/Axelrod-Python/Axelrod/jobs/217256127#L709 |
Have you rebased on to master? That's just a health check when hypothesis
can't find an example, I thought I'd fixed it by reducing the example size
but maybe not. I'll re kick off the build now (nothing that's the fault of
your patch).
…On Fri, 31 Mar 2017, 18:42 Julie Rymer, ***@***.***> wrote:
Ok so now I have another problem on python 3.5 :
hypothesis.errors.FailedHealthCheck: Data generation is extremely slow: Only produced 6 valid examples in 1.39 seconds (0 invalid ones and 0 exceeded maximum size). Try decreasing size of the data you're generating (with e.g.average_size or max_leaves parameters).
For those :
ERROR: test_decorator_with_stochastic_strategies (axelrod.tests.unit.test_property.TestProbEndSpatialTournament)
ERROR: test_decorator_with_stochastic_strategies (axelrod.tests.unit.test_property.TestProbEndTournament)
ERROR: test_decorator_with_stochastic_strategies (axelrod.tests.unit.test_property.TestSpatialTournament)
ERROR: test_property_serial_play (axelrod.tests.unit.test_tournament.TestTournament)
https://travis-ci.org/Axelrod-Python/Axelrod/jobs/217256127#L709
Apparently the error is caused by the call to the function associated with
the hypothesis.given decorator.
Is it a time performance test ?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#953 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ACCGWgAwcm-b1d8WcIrv0xUjIeGETZFQks5rrTsXgaJpZM4Mv0w7>
.
|
Also, have you seen the paremterized player test on #952? I realise I'm not answering your question about that fully, I'll take a closer look later this evening (just taking the 🐕 for a walk :)) |
Yes I'm doing fetch/merge regularly. |
Damn... That means my fix aiming to remove that timeout for it didn't work... I'll wait and see if it happens again... |
In >>235 @given(tournament=prob_end_spatial_tournaments(strategies=stochastic_strategies,
236 max_size=3,
~ 237 min_prob_end=0.7)) That might help this stop happening... |
I would prefer the same format of |
(Sorry about this, it's obviously not your fault and just a thing that happens with Hypothesis. Hopefully that'll fix it but I'll keep an eye on it.) |
Actually! Sorry for all these messages @Chadys! New suggestion. Remove all of these lines: 235 @given(tournament=prob_end_spatial_tournaments(strategies=stochastic_strategies,
236 max_size=3,
237 min_prob_end=0.7))
238 @settings(max_examples=10, timeout=0)
239 def test_decorator_with_stochastic_strategies(self, tournament):
240 self.assertIsInstance(tournament, axelrod.ProbEndSpatialTournament)
241 stochastic_player_names = [str(s()) for s in stochastic_strategies]
242 for p in tournament.players:
243 self.assertIn(str(p), stochastic_player_names) Coverage will not be affected and it's not at all a necessary test. |
Scrap, all that: sending you a PR to this branch :) |
Due to timeouts on travis as discussed on Axelrod-Python#953. - Reduced size in tournament tests. - Removed some tests for the property based tests themselves (this where not necessary and just tested a specific subset of strategies).
I did the merge and made |
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.
Looks good overall, just a few questions about some of the changes.
axelrod/player.py
Outdated
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: |
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.
axelrod/player.py
Outdated
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 comment
The reason will be displayed to describe this comment to others. Learn more.
Technically strategies won't have unique names since Random(0.12)
and Random(0.121)
will get the same name. Are we all ok with that?
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 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 comment
The reason will be displayed to describe this comment to others. Learn more.
Originally, the __repr__
of Random was doing a round. When I print all strategies __repr__
with default values I get srings a bit long for those :
ZD-Extort-2: 0.1111111111111111, 0.5
ZD-Extort-2 v2: 0.125, 0.5, 1
ZD-Extort-4: 0.23529411764705882, 0.25, 1
So maybe use a bigger value for round or, if you prefer to be really precise and you don't mind __repr__
like my example above, I can suppress it
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 suppose ultimately the floats in the __repr__
are being truncated anyway. I'm relatively indifferent between:
- Leaving without any rounding (or in essence using the default rounding of a python float);
- Perhaps rounding to 4 digits?
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 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 comment
The reason will be displayed to describe this comment to others. Learn more.
👍
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 comment
The reason will be displayed to describe this comment to others. Learn more.
This is the default __repr__
from Player
, yes? If so I think we don't need to add it again explicitly.
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.
When I don't override __repr__
here I get that :
Soft Go By Majority: inf, True
Soft Go By Majority: 10
Soft Go By Majority: 20
Soft Go By Majority: 40
Soft Go By Majority: 5
...
Hard Go By Majority: inf
Hard Go By Majority: 10
Hard Go By Majority: 20
Hard Go By Majority: 40
Hard Go By Majority: 5
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 comment
The reason will be displayed to describe this comment to others. Learn more.
'Go By Majority: True' or 'Go By Majority: False', are we okay with that ?
I'm fine with that.
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 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 comment
The 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 __repr__
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 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 comment
The 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 comment
The 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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps it's not needed for the base one.
classifier = _test_classifier | ||
|
||
def __init__(self, arg_test1='testing1', arg_test2='testing2'): | ||
super().__init__() |
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.
Do we not need to pass the arguments through to the super init?
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.
No since it calls Player.__init__
which takes no argument
@@ -270,7 +307,7 @@ def attribute_equality_test(self, player, clone): | |||
def test_clone(self): | |||
# Test that the cloned player produces identical play | |||
player1 = self.player() | |||
if str(player1) in ["Darwin", "Human"]: | |||
if str(player1)[:6] in ["Darwin", "Human:"]: |
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 does Human have a :
now?
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 Human now get its str in the form 'Human: human_name, symbol1, symbol2' so I can't test the exact name so I did a split and I needed the same number of letters for Darwin and Human, hence the "Human:". But I could replace that line by :
if player1.name in ["Darwin", "Human"]:
it is more explicit.
But that got me to identify a little bug :
In [5]: h= axl.Human('coco')
In [6]: str(h)
Out[6]: 'coco: coco, C, D'
So the question is : Do we want str(h)
in that case to return 'Human: coco, C, D' or 'coco: C, D' ? I'll correct my code with the solution choosen
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'd vote for: Human: coco, C, D
.
#952 has been merged now (with the parametrized test player). |
I'll do a fetch/merge and correct conflicts later today :) |
This reverts commit 180e28c.
I did all the asked changes and merged with the latest version 😄 |
I'm taking @marcharper's approved review as an indication that this is good to go. 👍 |
Following the discussion in #802, I did the dynamic naming of strategies, while adding (and using) an
init_params
class method inPlayer
to get a dict of the init parameters of a strategy (where the values are either given or default one are used). I also fixed #947 while I was at it.