issue#844 backstabber.py #924

eric-s-s · 2017-03-22T13:49:58Z

added tests.

also refactored backstabber and fixed an off-by-one error.
#884

drvinceknight

Thanks for this @eric-s-s it looks good.

Requests for docstrings (even if just a line)
I'd like you to tweak the tests a bit. Moving the sub calls to their own test_... method so that they get recognised as individual tests by unittest. This just helps in the case of test failures.

Could you point out and expand on the off by 1 error that you've fixed? Just to make it clear please.

drvinceknight · 2017-03-22T14:50:54Z

axelrod/strategies/backstabber.py

+        return _backstabber_strategy(opponent)
+
+
+def _backstabber_strategy(opponent):


Could you add a docstring here please.

drvinceknight · 2017-03-22T14:54:21Z

axelrod/strategies/backstabber.py

+    return C
+
+
+def _alt_strategy(opponent):


Docstring please.

drvinceknight · 2017-03-22T14:54:27Z

axelrod/strategies/backstabber.py

+    return C
+
+
+def _opponent_triggers_alt_strategy(opponent):


Docstring please.

drvinceknight · 2017-03-22T14:54:32Z

axelrod/strategies/backstabber.py

+    return before_alt_strategy < rounds_opponent_played < after_alt_strategy
+
+
+def _opponent_defected_in_first_n_rounds(opponent, first_n_rounds):


Docstring please.

drvinceknight · 2017-03-22T14:56:57Z

axelrod/tests/strategies/test_backstabber.py

-
-        # Defects on rounds 199, and 200 no matter what
-        self.responses_test([C, D, D], [C] * 197, [C] * 197, length=200)
+        self._when_alt_strategy_is_triggered()


Could you put these as their own test methods.

So remove them from here and rename them to start with the word test:

_when_alt_stragegy_is_triggered -> test_when_alt_strategy_is_triggered ...

This would make the test failures easier to read (by having them as specific tests instances so to speak).

drvinceknight · 2017-03-22T14:59:02Z

axelrod/tests/strategies/test_backstabber.py

@@ -24,58 +24,100 @@ def test_strategy(self):
        Forgives the first 3 defections but on the fourth
        will defect forever. Defects after the 198th round unconditionally.
        """
+        self._defects_after_four_defections()


Remove these and change the name of the methods to be test_defects_after_four_defections.

is ok that there will be no "test_strategy" method? it will get replaced by the more specifically named ones.

Yup that's fine, you'll still run test_strategy from the super TestBackStabber class so it's kind of like what you had before :)

drvinceknight · 2017-03-22T14:59:15Z

axelrod/tests/strategies/test_backstabber.py

+        alternator_actions = [(C, C), (C, D)] * 4 + [(D, C), (D, D)] * 2
+        self.versus_test(axelrod.Alternator(), expected_actions=alternator_actions, match_attributes={"length": 200})
+
+    def _defects_on_last_two_rounds_by_match_len(self):


change to test_defects...

drvinceknight · 2017-03-22T14:59:27Z

axelrod/tests/strategies/test_backstabber.py

+        self._alt_strategy_stops_at_round_180()
+        super(TestDoubleCrosser, self).test_strategy()
+
+    def _when_alt_strategy_is_triggered(self):


test_when_...

drvinceknight · 2017-03-22T14:59:39Z

axelrod/tests/strategies/test_backstabber.py

+        self.versus_test(axelrod.MockPlayer(opponent_actions), expected_actions=expected_actions,
+                         match_attributes={"length": 200})
+
+    def _starting_defect_keeps_alt_strategy_from_triggering(self):


test_starting_...

drvinceknight · 2017-03-22T14:59:52Z

axelrod/tests/strategies/test_backstabber.py

+                         expected_actions=defects_on_last_actions + expected_actions_suffix,
+                         match_attributes={"length": 200})
+
+    def _alt_strategy_stops_at_round_180(self):


test_alt_

eric-s-s · 2017-03-22T16:24:01Z

axelrod/strategies/backstabber.py

-            return C
-        if len(opponent.history) < 180:
-            if len(opponent.history) > cutoff:
-                if D not in opponent.history[:cutoff + 1]:


here's the off-by-one. It should only check for defection in the first 6, but opponent.history[:cutoff +1] is the first 7.

Thanks, looks like you are right to me. Would be good to have @uglyfruitcake's thoughts on this PR too :)

Yes the code doesn't match the documentation, however, It's the documentation that's wrong. I spent ages optimises the numbers in Doublecrosser to get it to perform optimally and its current state will be it's optimal performance. When I wrote the description I will have put the wrong number of turns and so it should say "the first 7 rounds". It is a mistake but its wrong in the documentation not the code. Sorry!

rewriting that now. in a moment, comments and tests should follow the specs.

eric-s-s · 2017-03-22T16:27:49Z

@drvinceknight thank you for all the comments. i'll get on it ASAP.

drvinceknight · 2017-03-22T17:23:39Z

axelrod/strategies/backstabber.py

+    """
+    before_alt_strategy = 6
+    after_alt_strategy = 180
+    if _opponent_defected_in_first_n_rounds(opponent, before_alt_strategy):


Can you explicitly write first_n_rounds=before_alt_strategy (for clarity).

eric-s-s · 2017-03-22T18:31:10Z

axelrod/strategies/backstabber.py

+    return C
+
+
+def _opponent_triggers_alt_strategy(opponent: Player) -> bool:


@uglyfruitcake is this correct?

Yes that looks good to me.

drvinceknight · 2017-03-23T08:44:15Z

axelrod/strategies/backstabber.py

+    return C
+
+
+def _alt_strategy(opponent: Player) -> Action:


Could you write specific tests for these module functions please.

The don't need to be extensive but just enough so that they'd fail if something got refactored in the strategy methods removing the need for them.

actually. this brings up a problem i had, to which module methods was the most expedient solution.

as i see it, these should all be private methods of their classes and DoubleCrosser should inherit from BackStabber. The tests, as they are, should fully cover all those methods (i don't have a coverage tool, but i think i got it).

I originally wrote class DoubleCrosser(BackStabber): , but strange things would happen with @FinalTransformer((D, D), name_prefix=None) If only BackStabber had that decorator, DoubleCrosser would not inherit that behavior. If both had that decorator I got a maximum recursion depth error.

that aside, will write tests for module as is.

errrr. i hope complete tests is ok. trying to write non-complete tests is, for me, like not completing a

Haha, this is way more than I was expecting but it looks good to me :)

drvinceknight · 2017-03-23T15:58:42Z

I've labelled this as ready to go but we have a 2 core reviewer process, so either @marcharper or @meatballs will be along when they have a moment and might make further requests (or indeed disagree with my requests :)) 👍

meatballs · 2017-03-23T16:51:05Z

axelrod/strategies/backstabber.py

+    """
+    If opponent's last two plays were defect, then defects on next round. Otherwise, cooperates.
+    """
+    final_two_plays = opponent.history[-2:]


Could we rename this variable? Given that a major feature of these strategies is to defect unconditionally for the final two rounds of the match, it's confusing to call this variable final_two_plays

How about previous_two_plays instead?

eric-s-s added 3 commits March 22, 2017 13:31

total refactor

d447b0b

total refactor

ea05b78

more thorough test

8385eef

eric-s-s mentioned this pull request Mar 22, 2017

Refactor all strategy tests #884

Closed

drvinceknight requested changes Mar 22, 2017

View reviewed changes

eric-s-s commented Mar 22, 2017

View reviewed changes

made suggested changes

54c4099

drvinceknight requested changes Mar 22, 2017

View reviewed changes

rewrote alt_strategy trigger test. and tests

f394247

eric-s-s commented Mar 22, 2017

View reviewed changes

little change

a240da1

drvinceknight requested changes Mar 23, 2017

View reviewed changes

unittests for module methods

3d3a27c

drvinceknight approved these changes Mar 23, 2017

View reviewed changes

drvinceknight added the ready-to-merge label Mar 23, 2017

meatballs requested changes Mar 23, 2017

View reviewed changes

meatballs added ready-for-review and removed ready-to-merge labels Mar 23, 2017

last_two to previous_two

5d74154

meatballs approved these changes Mar 23, 2017

View reviewed changes

meatballs added ready-to-merge and removed ready-for-review labels Mar 23, 2017

drvinceknight merged commit d6b171d into Axelrod-Python:master Mar 23, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

issue#844 backstabber.py #924

issue#844 backstabber.py #924

eric-s-s commented Mar 22, 2017 •

edited

drvinceknight left a comment

drvinceknight Mar 22, 2017

drvinceknight Mar 22, 2017

drvinceknight Mar 22, 2017

drvinceknight Mar 22, 2017

drvinceknight Mar 22, 2017

drvinceknight Mar 22, 2017

eric-s-s Mar 22, 2017

drvinceknight Mar 22, 2017

drvinceknight Mar 22, 2017

drvinceknight Mar 22, 2017

drvinceknight Mar 22, 2017

drvinceknight Mar 22, 2017

eric-s-s Mar 22, 2017

drvinceknight Mar 22, 2017

uglyfruitcake Mar 22, 2017

eric-s-s Mar 22, 2017

eric-s-s commented Mar 22, 2017

drvinceknight Mar 22, 2017

eric-s-s Mar 22, 2017

uglyfruitcake Mar 22, 2017

drvinceknight Mar 23, 2017

eric-s-s Mar 23, 2017

eric-s-s Mar 23, 2017

eric-s-s Mar 23, 2017

drvinceknight Mar 23, 2017

drvinceknight commented Mar 23, 2017

meatballs Mar 23, 2017

		return _backstabber_strategy(opponent)


		def _backstabber_strategy(opponent):

		return before_alt_strategy < rounds_opponent_played < after_alt_strategy


		def _opponent_defected_in_first_n_rounds(opponent, first_n_rounds):

		return C


		def _opponent_triggers_alt_strategy(opponent: Player) -> bool:

issue#844 backstabber.py #924

issue#844 backstabber.py #924

Conversation

eric-s-s commented Mar 22, 2017 • edited

drvinceknight left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eric-s-s commented Mar 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drvinceknight commented Mar 23, 2017

Choose a reason for hiding this comment

eric-s-s commented Mar 22, 2017 •

edited