Implement an eq method for players #975

drvinceknight · 2017-04-16T17:32:35Z

Add equality method and tests to player.
Add documentation for player equality.
Refactored some other tests to make use of this method
Refactor a match generator test which was failing as a result of the
eq method. Previously this was testing equality of instance.

Some other minor changes I've made as a result of this:

Change when qlearner makes random choice. I've also removed the reset
test. This is now being tested by the parent test class for all
strategies.
Fix small bug in contrite. Without this, contrite was not resetting
correctly (the recorded history was being maintained in the class
attribute).

This closes #885 but also closes #940 which was worked on a while ago
but which has not seen any progress.

marcharper · 2017-04-17T02:32:24Z

axelrod/player.py

+                setattr(other, attribute,
+                        (ele for ele in original_other_value))
+
+                if not (all(next(generator) == next(other_generator)


This could cause unintended behavior if used during a match or tournament.

I don't see what you mean?

I've done this in such a way that the generator gets copied and there are tests that show that using __eq__ does not alter the generator.

lines 199 and 215:

187 def test_equality_for_generator(self): 188 # Check generator attribute (a special case) 189 p1 = axelrod.Cooperator() 190 p2 = axelrod.Cooperator() 191 # Check that the generator is still the same 192 p1.generator = (i for i in range(10)) 193 p2.generator = (i for i in range(10)) 194 self.assertEqual(p1, p2) 195 196 _ = next(p2.generator) 197 self.assertNotEqual(p1, p2) 198 199 # Check that internal generator object has not been changed 200 self.assertEqual(list(p1.generator), list(range(10))) 201 self.assertEqual(list(p2.generator), list(range(1, 10))) 202 203 def test_equality_for_cycle(self): 204 # Check cycle attribute (a special case) 205 p1 = axelrod.Cooperator() 206 p2 = axelrod.Cooperator() 207 # Check that the cycle is still the same 208 p1.cycle = itertools.cycle(range(10)) 209 p2.cycle = itertools.cycle(range(10)) 210 self.assertEqual(p1, p2) 211 212 _ = next(p2.cycle) 213 self.assertNotEqual(p1, p2) 214 215 # Check that internal generator object has not been changed 216 self.assertEqual(next(p1.cycle), 0) 217 self.assertEqual(next(p2.cycle), 1)

So when the generator is copied it's state is also reset?

Yup that's right. What itertools.tee does is essentially fork a generator in to two versions of itself from the state it currently is in.

That is also being tests:

Create two Cooperators and give them two cycle generators and tests that they are equal (at this stage their generator are in the same state:

205 p1 = axelrod.Cooperator() 206 p2 = axelrod.Cooperator() 207 # Check that the cycle is still the same 208 p1.cycle = itertools.cycle(range(10)) 209 p2.cycle = itertools.cycle(range(10)) 210 self.assertEqual(p1, p2)

Bump p2s cycle and now the players are not the same:

212 _ = next(p2.cycle) 213 self.assertNotEqual(p1, p2)

Check what the next values in the cycle are for each player, for p1 it should be 0 because the player has not iterated through any values and for p2 it should be 1 because it has iterated through the first value:

215 # Check that internal generator object has not been changed 216 self.assertEqual(next(p1.cycle), 0) 217 self.assertEqual(next(p2.cycle), 1)

Let me know if you think I could clarify that in the code and/or add other tests.

FYI test_equality_on_init is the test that fails in this case.

This should clarify why:

>>> import itertools >>> import types >>> cycle = itertools.cycle('ABCD') >>> new_cycle, backup = itertools.tee(cycle) >>> type(cycle), type(new_cycle), type(backup) (itertools.cycle, itertools._tee, itertools._tee) >>> isinstance(cycle, itertools.cycle), isinstance(cycle, types.GeneratorType) (True, False) >>> isinstance(new_cycle, itertools.cycle), isinstance(backup, types.GeneratorType) (False, False)

So the second time around the generator/cycle is not compared in the correct way. I did have a special case for a cycle but found it more elegant to just treat them all the same way. :)

We could add isinstance(value, collections.Iterable) to the first check but that would also catch lists etc which we don't want :)

I did have a special case for a cycle but found it more elegant to just treat them all the same way.

This might actually be a better way as it ensures we don't change anything at all (even the type of the attributes). I've also realised something else that's not right in this test. Going to tweak and push.

In c5e95d6 I've added a third equality test (theoretically things could break on the second one). I've also added test that ensure the type of a generator/cycle stays the same (which implies I've changed the way I've dealt with the output of itertools.tee). 👍 Let me know what you think :)

Thanks for the explanations!

marcharper · 2017-04-17T02:33:48Z

axelrod/tests/strategies/test_player.py

+        p1 = axelrod.Cooperator()
+        p2 = axelrod.Cooperator()
+        self.assertEqual(p1, p2)
+        p1.__repr__ = lambda :"John Nash"


- Add equality method and tests to player. - Add documentation for player equality. - Refactored some other tests to make use of this method - Refactor a match generator test which was failing as a result of the eq method. Previously this was testing equality of instance. Some other minor changes I've made as a result of this: - Change when qlearner makes random choice. I've also removed the reset test. This is now being tested by the parent test class for all strategies. - Fix small bug in contrite. Without this, contrite was not resetting correctly (the recorded history was being maintained in the class attribute). This closes #885 but also closes #940 which was worked on a while ago but which has not seen any progress.

marcharper · 2017-04-18T00:28:48Z

I approve. One comment for thought -- it seems possible to get into an infinite loop if player A contains an attribute referencing player B and player B contains a reverse reference. I couldn't think of a legitimate reason to do so -- even with the meta players we don't let them be team members of other meta players -- but maybe worth some thought for the possibility of other cases.

drvinceknight · 2017-04-18T08:46:22Z

That's a really good point. Let me have a play with a couple of ideas I've got. Will push/comment later today.

Check equality of players when players have attributes that point at each other. Also included some more tests for basic attribute equality.

drvinceknight · 2017-04-18T18:44:16Z

I've covered this with: eb612f5

Have added a bunch of tests that check we get what we want :)

(I also added a couple of other specific tests for some cases I thought were worth checking :))

drvinceknight · 2017-04-18T20:12:58Z

Just a small typo on the word equality ;)

Thanks 👍 :)

marcharper · 2017-04-19T00:05:10Z

@meatballs you're up when you get a chance

This was referenced Apr 16, 2017

Refactor the static methods to functions in fingerprint #974

Closed

885 #940

Closed

drvinceknight force-pushed the 885 branch from 6f472b4 to 3db1eac Compare April 16, 2017 20:36

marcharper requested changes Apr 17, 2017

View reviewed changes

drvinceknight added 5 commits April 17, 2017 17:51

Add missing type annotations for qlearner.

819b618

Remove unneeded method overwrite.

77b29cc

Fix spacing.

caf2783

Add inline comments and docstring.

308741e

drvinceknight force-pushed the 885 branch from 95f0c21 to 308741e Compare April 17, 2017 16:55

Ensure types of generator/cycle stay the same.

c5e95d6

marcharper approved these changes Apr 18, 2017

View reviewed changes

Add eq for very strange edge case.

eb612f5

Check equality of players when players have attributes that point at each other. Also included some more tests for basic attribute equality.

Fix typo.

239a511

marcharper added the ready-to-merge label Apr 19, 2017

meatballs merged commit 12efc34 into master Apr 19, 2017

meatballs deleted the 885 branch April 19, 2017 09:04

Implement an eq method for players #975

Implement an eq method for players #975

Uh oh!

Conversation

drvinceknight commented Apr 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcharper commented Apr 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

drvinceknight commented Apr 18, 2017

Uh oh!

drvinceknight commented Apr 18, 2017

Uh oh!

drvinceknight commented Apr 18, 2017

Uh oh!

marcharper commented Apr 19, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

marcharper commented Apr 18, 2017 •

edited

Loading