-
Notifications
You must be signed in to change notification settings - Fork 263
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
ML strategies #803
ML strategies #803
Conversation
Oooof this is a biggy! |
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 a first shallow sweep, there's a lot here: could take me a little while but it looks like awesome work!
""" | ||
|
||
name = 'Worse and Worse' | ||
classifier = { | ||
'memory_depth': float('inf'), |
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 needs to know the current turn so does that not equate to knowing how long the game has been? (Thus infinite memory).
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'm actually not sure about this one so I've changed it back to float('inf')
for now. The question is whether using the round number counts as using history or not, let's discuss elsewhere.
@@ -0,0 +1,123 @@ | |||
"""Tests for Finite State Machine Strategies.""" |
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.
hidden markov model strategies
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.
done
@@ -22,6 +22,10 @@ documentation. | |||
.. [Li2009] Li, J. & Kendall, G. (2009). A Strategy with Novel Evolutionary Features for the Iterated Prisoner’s Dilemma. Evolutionary Computation 17(2): 257–274. | |||
.. [Li2011] Li, J., Hingston, P., Member, S., & Kendall, G. (2011). Engineering Design of Strategies for Winning Iterated Prisoner ’ s Dilemma Competitions, 3(4), 348–360. | |||
.. [Li2014] Li, J. and Kendall, G. (2016). The Effect of Memory Size on the Evolutionary Stability of Strategies in Iterated Prisoner's Dilemma. IEEE Transactions on Evolutionary Computation, 18(6) 819-826 | |||
.. [Mathieu2015] Mathieu, P. and Delahaye, J. New Winning Strategies |
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 the docs build does this look right? (Just doesn't match the one line formatting used for all others).
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.
Citation updated
url='http://axelrod.readthedocs.org/', | ||
license='The MIT License (MIT)', | ||
description='Reproduce the Axelrod iterated prisoners dilemma tournament', | ||
include_package_data=True, |
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.
If you haven't, could you check that this pip installs? (eg pip installing in to a virtual env from the local dir)
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 locally, and the tests would fail on travis otherwise.
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 don't think travis specifically tests this (just fyi) but appveyor does test the setup install (because we had some windows problems with that at some point...).
@@ -0,0 +1,45 @@ | |||
import pkg_resources |
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 have unit tests for these please.
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.
Not sure how to better test these than simply loading the data and testing the strategies. I added a few integrity checks in ANN and LookerUp to make sure the data is of the expected length.
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 was thinking that we could have a tests/unit/test_load_data.py
file that just checks that these functions run and that the data is of the expected format separately.
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'm not sure that we'd be testing anything further in that case -- if the format or data types are wrong the strategies will fail when constructed or played.
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 agree that this wouldn't test anything further, it just consolidates things: for example in the future these ml strategies could be changed to no longer read the data (hypothetically), their tests adjusted and an error creeping in to these reader functions. That's a weird case but I think my point holds?
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.
But if the players no longer read the data then the data and these functions are unnecessary (and their coverage will disappear). So wouldn't we just delete the data and these functions in that case, unless something else is using them? And if something else is using them then a bad change will still break those things.
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.
Very good point, I'd still say too many tests is better than too phew but I won't insist. :) 👍
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.
Maybe type annotations are a good check here so when start annotating we'll get a little extra coverage.
"""Implementation of a basic Hidden Markov Model. We assume that the | ||
transition matrix is conditioned on the opponent's last action, so there | ||
are two transition matrices. Emission distributions are stored as Bernoulli | ||
probabilities for each state. This is essentially a stochastic FSM. |
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.
Names
- SimpleHMM: ...
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 one isn't a strategy but I updated the HMM Players with names
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.
Sorry I got over zealous :)
|
||
|
||
class HMMPlayer(Player): | ||
"""Abstract base class for Hidden Markov Model players.""" |
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.
Same comment names ...
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.
done
self.hmm.state = self.initial_state | ||
|
||
|
||
class EvolvedHMM5(HMMPlayer): |
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.
names
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.
done
@@ -14,7 +14,7 @@ class TestEvolvedANN(TestPlayer): | |||
expected_classifier = { | |||
'memory_depth': float('inf'), | |||
'stochastic': False, | |||
'makes_use_of': set(["length"]), |
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.
Is the number of turns no longer a feature?
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.
Nope! Just the round number.
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.
Cool :)
def test_malformed_params(self): | ||
# Test a malformed table | ||
t_C = [[1, 0.5], [0, 1]] | ||
self.assertFalse(is_stochastic_matrix(t_C)) |
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.
Re my request for a test for this function, could it be pulled out of here and tested independently? (With a True assertion as well).
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.
done
@@ -0,0 +1,5 @@ | |||
# name, features, hidden_layer_size, weights... |
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.
Small suggestion: remove the #
and list full header (potentially useful for other analysis?). I expect this would need to be done on the axelrod-evolver
repo and not for this PR.
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 cross this bridge later. The number of columns isn't constant so there isn't really a proper header.
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.
Fine to leave for later, you could have the max number of columns with headers and have NANs in the other ones? (Not suggesting that for 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.
This is a fantastic addition to the library @marcharper :)
My requests are mainly docstrings and more tests as well as a couple of questions.
import pkg_resources | ||
|
||
|
||
def load_file(filename, directory): |
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.
Docstring for completeness. Also numpy style for this and the rest?
|
||
If the mutation_rate is 0, the population will eventually fixate on | ||
exactly one player type. In this case a StopIteration exception is | ||
raised and the play stops. If mutation_rate is not zero, then the |
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 mutation_rate
hidden_layer_size | ||
) | ||
ANN.__init__(self, i2h, h2o, bias) | ||
num_features, num_hidden, weights = nn_weights['1'] |
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 change the 1
in the dataset and here to be 10
? This would be for readability and corresponding to the size of the hidden layer.
|
||
Names: | ||
|
||
- EvolvedANN5: : Original name by Marc Harper. |
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.
Extra :
@@ -158,7 +160,8 @@ def strategy(self, opponent): | |||
|
|||
class EvolvedANN(ANN): | |||
""" | |||
A strategy based on a pre-trained neural network. | |||
A strategy based on a pre-trained neural network with 17 features and a |
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.
Would it be worth including a bullet point list of the 17 features here?
for m in [self.hmm.transitions_C, self.hmm.transitions_D]: | ||
for row in m: | ||
values.update(row) | ||
if not values.issubset({0, 1}): |
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.
return not values.issubset({0, 1})
.
(This is stylistic, I don't feel strongly about it.)
|
||
Names | ||
|
||
- EvolvedHMM5 |
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.
: Original name...
player = self.player([[1]], [[1]], [0], initial_state=0) | ||
player.hmm.state = -1 | ||
player.reset() | ||
self.assertFalse(player.hmm.state == -1) |
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 have a specific test for the EvolvedHMM5
.
player = self.player([[1]], [[1]], [0], initial_state=0) | ||
player.hmm.state = -1 | ||
player.reset() | ||
self.assertFalse(player.hmm.state == -1) |
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 assertNotEqual
would be better here.
And/or even assertEqual
with 0.
@@ -19,7 +19,7 @@ class MetaPlayer(Player): | |||
classifier = { | |||
'memory_depth': float('inf'), # Long memory | |||
'stochastic': True, | |||
'makes_use_of': set(), | |||
'makes_use_of': {'game', 'length'}, |
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 you walk me through this one please?
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 default player set has members that use both the game and the match length.
import pkg_resources | ||
|
||
|
||
def load_file(filename, directory): |
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.
Wouldn't these be better as a set of pandas dataframes? There would be far less code and it would be quicker too.
I know it's another dependency, but we're already dependent on numpy, so the precedent has been set.
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.
Yep! I prefer using pandas actually if the extra dependency is ok with @drvinceknight .
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'm not averse to the extra dependency. 👍
Could change the output of the results_set.summarize
to be a data frame too (not for this PR, another issue :)).
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.
Cool. I think anyone that uses anaconda or can pip install numpy should have access to rest of the scientific stack (for sure at least scipy and pandas).
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'm punting on this one since the number of columns isn't constant in all 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.
Fine by me.
I'm not entirely sure use dfs would make things simpler in this case, the data as is would need to be pivoted for the df to be advantageous or the data could be stored with rows corresponding to "genes" and columns to different strategies... Perhaps not a bad idea (but I don't think necessary for this PR).
db87d10
to
a1a4969
Compare
a1a4969
to
17abaaf
Compare
I believe that I addressed all the comments sufficiently, let me know if you agree! |
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.
All looks good to me! Nice job :)
This PR adds several new strategies.
Of particular note:
It's likely that these strategy are not the best possible, and that better versions can be evolved in the future. I trained many strategies, some of which are not included, particularly:
There are also other conceivable training modes, e.g. maximum total score (self + opponent). The point is: we may decide to add or remove trained strategies in the future.
Also in this PR: