Fixes #31 (fix lookup_evolve) #50
Conversation
|
Thanks! I presume you ran it locally since it looks like we didn't have a broken test. |
|
Yes -- due to limited compute I ran it like this:
which yields |
| @@ -104,7 +104,7 @@ def mutate(self): | |||
|
|
|||
| @staticmethod | |||
| def crossover_tables(table1, table2): | |||
| keys = list(sorted(table1.keys())) | |||
| keys = list(table1.keys()) | |||
There was a problem hiding this comment.
I think we still want the sort here, or is there a good reason to remove it?
There was a problem hiding this comment.
I removed it because it was throwing this error:
TypeError: '<' not supported between instances of 'Action' and 'Action'
I considered defining a total order on the Action class, but I don't think sorting is actually needed here. The keys are considered in the same order for both table1 and table2, so the crosspoint still defines the same partition of the keys for both tables, which seems sufficient.
bin/lookup_evolve.py
Outdated
| new_table) | ||
|
|
||
| def __repr__(self): | ||
| return "{}:{}:{}:{}:{}".format( | ||
| self.plays, | ||
| self.op_plays, | ||
| self.op_start_plays, | ||
| ''.join(self.initial_actions), | ||
| ''.join([v for k, v in sorted(self.table.items())]) | ||
| self.initial_actions, |
There was a problem hiding this comment.
actions_to_str here as well?
There was a problem hiding this comment.
Done, please see below.
|
Looks good, just two questions. |
bin/lookup_evolve.py
Outdated
| ''.join(self.initial_actions), | ||
| ''.join([v for k, v in sorted(self.table.items())]) | ||
| actions_to_str(self.initial_actions), | ||
| actions_to_str([v for k, v in self.table.items()]) |
There was a problem hiding this comment.
I think we need to sort these guys (via actions like you suggested above). Otherwise these representations won't be unique, and reversing __repr__ won't work in a stable manner.
There was a problem hiding this comment.
Once Axelrod-Python/Axelrod#1199 is in and a new release of Axelrod done (I'll do it as soon as) we could bump https://github.com/Axelrod-Python/axelrod-dojo/blob/master/requirements.txt to keep track of the fact that we need the ordered actions?
|
I added sorting back now that Axelrod-Python/Axelrod#1199 has been merged. Running
|
|
I've just released @jsafyan could you bump https://github.com/Axelrod-Python/axelrod-dojo/blob/master/requirements.txt#L1 to be: 👍 Side question: any trouble that CI passed even when it would have been using a version of the Axelrod library without ordered actions? |
We don't have a test that makes sure that the pattern representation is sorted, but we need it for reproducibility and reloading previously evolved strategies. |
|
LGTM (thanks @jsafyan 👍) |
Fixes #31 and changes
mutation_ratetomutation_probabilityto conform to the Population genetic algorithm class.