Dbs #976

edouardArgenson · 2017-04-16T22:17:41Z

Hello,
I have written code for one of the desired new strategies, (DBS, DesiredBeliefStrategy).
ref: http://www.cs.utexas.edu/%7Echiu/papers/Au06NoisyIPD.pdf

I've followed the algorithm description in the article, and used the same variable names. When some details where not specified I made a few tests to ensure I had a good implementation.

I've checked that it work as expected in some situations, and that it performs good in noisy tournaments as it should.

I've written and run tests, but I don't really know if those are sufficient. Strategy tests, unit tests and integration tests are working, but I get warnings with strategy test.

This is my first contribution to an open source project so I don't really know if I need to check or detail more things, please let me know if I'm missing some points or anything

marcharper · 2017-04-17T02:42:31Z

The failed test for Py 3.6 was due to hypothesis, just restarted...

marcharper

Thanks for the contribution! On the first pass there are some formatting issues, please address those and I'll make another pass with the reference to check all the logic.

marcharper · 2017-04-17T02:44:05Z

axelrod/strategies/dbs.py

+C, D = Actions.C, Actions.D
+
+def action_to_int(action):
+    return (1 if action==C else 0)


Suggestion for readability:

if action == C: return 1 return 0

Or use a dictionary:

d = {C: 1, D: 0} return d[action]

marcharper · 2017-04-17T02:45:05Z

axelrod/strategies/dbs.py

+class DBS(Player):
+    """
+    Desired Belief Strategy as described in:
+    Accident or Intention: That Is the Question (in the Noisy Iterated Prisoner's Dilemma) by Tsz-Chiu Au and Dana Nau from University of Maryland


Thanks for including the reference. We have a citation file and a specific format we prefer, can you try to convert? We can help...

Sure, the full citation is:
T.-C. Au and D. S. Nau. Accident or intention: That is the question (in the iterated prisoner’s dilemma). In Proc. Int. Conf. Auton. Agents and Multiagent Syst. (AAMAS), pp. 561–568, 2006
I guess it becomes [Au2006] in the specific format ? I'll add it to the bibliography.rst file and change the description

marcharper · 2017-04-17T02:45:30Z

axelrod/strategies/dbs.py

+    Accident or Intention: That Is the Question (in the Noisy Iterated Prisoner's Dilemma) by Tsz-Chiu Au and Dana Nau from University of Maryland
+    http://www.cs.utexas.edu/%7Echiu/papers/Au06NoisyIPD.pdf
+
+    A strategy that learns the opponent's strategy, and use symbolic noise de-


Can we wrap lines at 80 and not split words with hyphens?

marcharper · 2017-04-17T02:46:15Z

axelrod/strategies/dbs.py

+    Default values for the parameters are the suggested values in the article 
+    When more noise you can try to diminish violation_threshold and rejection_threshold
+
+    Parameters:


Please see our formatting for other strategies for the parameter docs (and thanks for including them)

marcharper · 2017-04-17T02:47:54Z

axelrod/strategies/dbs.py

+        # by the opponent; else G[i]=0
+        # F[i] = 1 if cond j was True at turn i-1; else G[i]=0
+        # initial hypothesized policy is TitForTat
+        self.history_by_cond[(C,C)]=([1],[1])


We use PEP8's recommended formatting. For this line it would be:
self.history_by_cond[(C, C)] = ([1], [1])
(and similarly below)

thanks, I'll review the code with PEP8's rules

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

axelrod/strategies/dbs.py

+        return MoveGen((self.history[-1],opponent.history[-1]),self.Pi,depth_search_tree=self.tree_depth)
+
+
+# Policy as defined in the article, i.e. a set of (last_move,p) where p is


Can we move the Policy class above the strategy?

marcharper · 2017-04-17T02:55:25Z

axelrod/strategies/dbs.py

+# Policy as defined in the article, i.e. a set of (last_move,p) where p is
+# the probability to cooperate in the next move considering last move
+class Policy(dict):
+


Can we write a good docstring and move the comments just above into the docstring?

marcharper · 2017-04-17T02:55:55Z

axelrod/strategies/dbs.py

+# the probability to cooperate in the next move considering last move
+class Policy(dict):
+
+    def __init__(self):


You can use the default __init__ in this case so these two lines are unnecessary

I have deleted the Policy class. Policies are now represented by a simple dictionary, and there is a create_policy methods to instantiate them.

marcharper · 2017-04-17T02:56:11Z

axelrod/strategies/dbs.py

+    @classmethod
+    def prob_policy(cls,pCC,pCD,pDC,pDD):
+        pol = cls()
+        pol[(C,C)]=pCC


pol[(C, C) = pCC

marcharper · 2017-04-17T02:57:11Z

axelrod/strategies/dbs.py

+            return 1
+
+# tree search function (minimax search procedure)
+def F(begin_node,policy,max_depth):


Let's give F a good name. If it's called F in the reference then let's say so in the doc string

drvinceknight · 2017-04-17T07:32:11Z

This looks great @edouardArgenson, thanks for the contribution. As well as all of @marcharper's comments this is also failing on coverage (which checks that every line of code in the source files are hit during testing). Currently the following lines are not tested:

133             return True

198                         self.Rd.update(self.Rc)                                                                                                   
199                         self.Rc.clear()                                                                                                           
200                         self.violation_counts.clear()                                                                                             
201                         self.v=0

260         raise NotImplementedError('subclasses must override get_siblings()!')

264         raise NotImplementedError('subclasses must override is_stochastic()!')

If we can assist with writing tests for those please let us know. 👍

…method create_policy that returns a dict

edouardArgenson · 2017-04-17T14:02:43Z

@drvinceknight ok I'll look for tests that cover those lines

…e mecanism

edouardArgenson · 2017-04-20T19:44:00Z

Hi @drvinceknight,

I added a test that covers the following:
133 return True
and
198 self.Rd.update(self.Rc)
199 self.Rc.clear()
200 self.violation_counts.clear()
201 self.v=0

But I have trouble writing a test for the last 2:
260 raise NotImplementedError('subclasses must override get_siblings()!')
and
264 raise NotImplementedError('subclasses must override is_stochastic()!')
Those come from two abstract methods in the Node class, which is an abstract class with two subclasses StochasticNode and DeterministNode where the methods are implemented.
I don't really know how to test those lines, because it would involve creating a third subclass of Node, in which the two methods are not implemented, hence raising the errors.
I would welcome some help on that !

edouardArgenson · 2017-04-20T19:51:19Z

@marcharper I've pushed some changes, the code should now respect PEP8 and the formatting issues you've raised

drvinceknight · 2017-04-20T20:11:18Z

I would welcome some help on that !

No problemo :) This is the kind of thing that would work:

 import axelrod                                                                                                                                       
+import unittest                                                                                                                                      
 from .test_player import TestPlayer                                                                                                                  
                                                                                                                                                      
 C, D = axelrod.Actions.C, axelrod.Actions.D                                                                                                          
                                                                                                                                                      
                                                                                                                                                      
+class TestNode(unittest.TestCase):                                                                                                                   
+    """                                                                                                                                              
+    Tests for the base class                                                                                                                         
+    """                                                                                                                                              
+    node = axelrod.dbs.Node()                                                                                                                        
+                                                                                                                                                     
+    def test_get_siblings(self):                                                                                                                     
+        with self.assertRaises(NotImplementedError) as context:                                                                                      
+            self.node.get_siblings()                                                                                                                 
+                                                                                                                                                     
+    def test_is_stochastic(self):                                                                                                                    
+        with self.assertRaises(NotImplementedError) as context:                                                                                      
+            self.node.is_stochastic()                                                                                                                
+                                                                                                                                                     
+                                                                                                                                                     
+

So there I'm suggesting testing the base abstract class directly. I've checked those lines and they bump the coverage up to 100% 👍

drvinceknight · 2017-04-20T20:30:50Z

There's a failure on the documentation.

This should be:

     def get_siblings(self, policy):                                                                                                             
         """                                                                                                                                     
         build 2 siblings :code:`(C, *)` and :code:`(D, *)`                                                                                      
         siblings of a DeterministicNode are Stochastic, and are of the                                                                          
         same depth                                                                                                                              
         """

The * is a special rst symbol so there I'm putting in an inline code block,

I also noticed:

class StochasticNode(Node):                                                                                                                     
     "Node that have a probability p to get to each sibling"                                                                                     
     "Nodes (C, *) or (D, *)"

Could you use """ there please.

edouardArgenson · 2017-04-20T21:43:15Z

I made some changes on that and pushed it
oh I forgot to change the test

meatballs · 2017-04-22T13:13:43Z

axelrod/strategies/dbs.py

+    Desired Belief Strategy as described in [Au2006]_
+    http://www.cs.utexas.edu/%7Echiu/papers/Au06NoisyIPD.pdf
+
+    A strategy that learns the opponent's strategy, and use symbolic 


typo: 'uses' rather than 'use'

meatballs · 2017-04-22T13:18:48Z

axelrod/strategies/dbs.py

+    article. When noise increases you can try to diminish 
+    violation_threshold and rejection_threshold
+
+    Parameters:


The docstring format for these parameters isn't quite correct. We use the numpy standard described at https://github.com/numpy/numpy/blob/master/doc/HOWTO_DOCUMENT.rst.txt

e.g. The first parameter description should be:

discount_factor: float used when computing discounted frequencies to learn opponent's strategy Must be between 0 and 1. The default is 0.75

meatballs · 2017-04-22T13:19:37Z

axelrod/strategies/dbs.py

+            opposite_action = 1
+        k = 1
+        count = 0
+        # We iterates on the history, while we do not encounter


typo: 'iterate' rather than 'iterates'

meatballs · 2017-04-22T13:19:57Z

axelrod/strategies/dbs.py

+        k = 1
+        count = 0
+        # We iterates on the history, while we do not encounter
+        # counter-exemples of r_plus, i.e. while we do not encounter


typo: 'example' rather than 'exemple'

meatballs · 2017-04-22T13:20:36Z

axelrod/strategies/dbs.py

+                        == opposite_action
+                    and self.history_by_cond[r_plus[0]][1][1:][-k] == 1)
+            ):
+            # We count every occurence of r_plus in history


typo: 'occurrence' rather than 'occurence'

meatballs · 2017-04-22T13:21:51Z

axelrod/strategies/dbs.py

+    def should_demote(self, r_minus, violation_threshold=4):
+        if(self.violation_counts[r_minus[0]] >= violation_threshold):
+            return True
+        return False


Suggestion: This could be done in one line with:

return(self.violation_counts[r_minus[0]] >= violation_threshold)

meatballs · 2017-04-22T13:25:45Z

axelrod/strategies/dbs.py

+    def is_stochastic(self):
+        return False
+
+    def get_value(self):


Suggestion: this might be better with a dict mapping a tuple of action1 and action2 to a value:

values = { (C, C): 3, (C, D): 0, (D, C): 5, (D, D): 1 } return values[(action1, action2)]

marcharper · 2017-04-26T00:20:32Z

axelrod/strategies/dbs.py

+
+    Parameters
+    ----------
+    discount_factor : float, optional


instead of optional, can we put in the defaults from init into the docstring?

The defaults from init are currently specified in the description of each parameters. I can change that, but here I followed the numpy standard suggested by meatballs https://github.com/numpy/numpy/blob/master/doc/HOWTO_DOCUMENT.rst.txt

marcharper · 2017-04-26T00:23:10Z

axelrod/strategies/dbs.py

+    where p is the probability to cooperate after prev_move,
+    where prev_move can be (C, C), (C, D), (D, C) or (D, D)
+    """
+    pol = {}


This is fine as is but you might consider:
pol = {(C, C): pCC, (C, D): pCD, (D, C): pDC, (D, D): pDD}

Yes it's cleaner

marcharper · 2017-04-26T00:25:04Z

axelrod/strategies/dbs.py

+        # The stochastic node value is the expected values of siblings
+        node_value = (
+            begin_node.pC * minimax_tree_search(
+                                            siblings[0], 


indent should be <---- (4 spaces deeper than the line above), or:

node_value = ( begin_node.pC * minimax_tree_search( siblings[0], policy, max_depth), ...

marcharper · 2017-04-26T00:25:19Z

axelrod/strategies/dbs.py

+                                            max_depth)
+            )
+        return node_value
+    else:   # determinist node


deterministic

marcharper · 2017-04-26T00:25:40Z

axelrod/strategies/dbs.py

+        return node_value
+    else:   # determinist node
+        if begin_node.depth == max_depth:
+            # this is an end node, we just return its outcome value


For comments please capitalize and use complete sentences when possible

marcharper · 2017-04-26T00:26:30Z

axelrod/tests/strategies/test_dbs.py

@@ -0,0 +1,108 @@
+"""Tests DBS strategy."""


Can we have tests for the tree search functions as well?

marcharper · 2017-04-26T00:28:47Z

axelrod/strategies/dbs.py

+
+        # default opponent's policy is TitForTat
+        self.Rd = create_policy(1, 1, 0, 0)
+        self.Rc = {}


For the short variables can you explain with a comment what they are? I assume these are from the paper but we don't want anyone reading the code to have to read the paper to understand the algorithm.

marcharper · 2017-04-26T00:29:08Z

axelrod/strategies/dbs.py

+        self.violation_threshold = violation_threshold
+        self.promotion_threshold = promotion_threshold
+        self.tree_depth = tree_depth
+        self.v = 0


Please add a comment explaining the the variable v

marcharper · 2017-04-26T00:29:49Z

axelrod/strategies/dbs.py

+        self.history_by_cond[(D, D)] = ([0], [1])
+
+    def should_promote(self, r_plus, promotion_threshold=3):
+        if r_plus[1] == C:


Function needs a docstring

marcharper · 2017-04-26T00:30:05Z

axelrod/strategies/dbs.py

+        return False
+
+    def should_demote(self, r_minus, violation_threshold=4):
+        return (self.violation_counts[r_minus[0]] >= violation_threshold)


marcharper · 2017-04-26T00:30:11Z

axelrod/strategies/dbs.py

+    def should_demote(self, r_minus, violation_threshold=4):
+        return (self.violation_counts[r_minus[0]] >= violation_threshold)
+
+    def update_history_by_cond(self, opponent_history):


marcharper · 2017-04-26T00:30:18Z

axelrod/strategies/dbs.py

+                F.append(0)
+
+    def compute_prob_rule(self, outcome, alpha):
+        G = self.history_by_cond[outcome][0]


marcharper · 2017-04-26T00:31:17Z

axelrod/strategies/dbs.py

+            if (self.history_by_cond[r_plus[0]][1][1:][-k] == 1):
+                count += 1
+            k += 1
+        if(count >= promotion_threshold):


space after if

marcharper · 2017-04-26T00:32:08Z

axelrod/strategies/dbs.py

+        F = self.history_by_cond[outcome][1]
+        discounted_g = 0
+        discounted_f = 0
+        alpha_k = 1


does alpha_k ever change? Seems redundant below if it is always equal to 1

Yes alpha_k is iterated in the loop:
alpha_k = alpha * alpha_k

marcharper · 2017-04-26T00:32:23Z

axelrod/strategies/dbs.py

+        discounted_f = 0
+        alpha_k = 1
+        for g,f in zip(G[::-1], F[::-1]):
+            discounted_g += alpha_k*g


spaces around operators please

marcharper · 2017-04-26T00:33:22Z

axelrod/strategies/dbs.py

+        super().__init__()
+
+        # default opponent's policy is TitForTat
+        self.Rd = create_policy(1, 1, 0, 0)


We've called policies "four vectors" elsewhere (in strategies/memoryone.py), are they always dictionaries of size 4?

They can be of size 0, 1, 2, 3 or 4

A question: maybe I should put the definition of create_policy inside the DBS class, as it is only used inside the class ?

marcharper · 2017-04-26T00:34:32Z

axelrod/strategies/dbs.py

+        return True
+
+
+class DeterministNode(Node):


Please rename to DeterministicNode

marcharper · 2017-04-26T00:35:37Z

Making good progress -- it needs more comments and more tests, and there are some minor style issues.

drvinceknight · 2017-04-27T07:37:52Z

axelrod/strategies/dbs.py

+    violation_threshold and rejection_threshold
+
+    Parameters
+    ----------


The tests are failing because we don't quite use numpy notation exactly for strategies. In this case we need to remove the heading. So not:

Parameters ----------------

but simply

Parameters

edouardArgenson · 2017-04-27T14:08:47Z

Thanks, working on all that

…e to DeterministicNode

…rmats comments and parameters

… and is_stochastic functions. Corrects syntax error when initializing history_by_cond in constructor

drvinceknight · 2017-05-01T20:51:21Z

@edouardArgenson there is now a merge conflict in the bibliography.rst file with the master branch. Let us know if you need a hand resolving that :)

edouardArgenson · 2017-05-01T22:14:50Z

merged conflict resolved ;)

edouardArgenson added 5 commits April 15, 2017 20:47

adds dbs.py in strategies repository

3be6d6c

adds test file, add reset method

cc168f4

adds new module to docs/references/all_strategies.rst file

68b47f6

clean debbugging prints

da4d686

add precisions on dbs description

d77383d

marcharper requested changes Apr 17, 2017

View reviewed changes

edouardArgenson added 5 commits April 17, 2017 12:47

adds reference to bibliography.rst

6ed2688

adapt bibliography format in strategy description

46aad13

apply PEP8 recommendations to code

2182512

replace Policy class by instanciations of dictionnaries, only keep a …

70a0b1c

…method create_policy that returns a dict

rename method F as minimax_search_procedure

d6dbef6

edouardArgenson added 3 commits April 20, 2017 19:34

readability modifications

a152100

corrects name error in minimax_tree_search function

1bc6a34

adds test to increase coverage, in particular coverage of ShouldDemot…

d5fbd47

…e mecanism

changes in Node documentation to avoid failure on doc

9f4f3ff

edouardArgenson added 2 commits April 21, 2017 00:39

adds test for Node NotImplementedError

8559ef2

correct little mistakes and ensure it passes the tests

10489ae

meatballs requested changes Apr 22, 2017

View reviewed changes

edouardArgenson added 3 commits April 25, 2017 18:39

corrects typo mistakes and make some readability changes

bc03a2d

corrects docstring format for parameters

6562eca

corrects spelling mistake in comments

fc1ff03

marcharper reviewed Apr 26, 2017

View reviewed changes

axelrod/strategies/dbs.py Outdated

max_depth)

)

return node_value

else: # determinist node

Copy link

Member

marcharper Apr 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

deterministic

marcharper reviewed Apr 26, 2017

View reviewed changes

axelrod/strategies/dbs.py Outdated

return True

class DeterministNode(Node):

Copy link

Member

marcharper Apr 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rename to DeterministicNode

drvinceknight requested changes Apr 27, 2017

View reviewed changes

edouardArgenson added 3 commits April 27, 2017 17:02

corrects minor formatting issues and change class name DeterministNod…

d796578

…e to DeterministicNode

adds docstrings for functions. adds comments describing variables. fo…

2a60d7d

…rmats comments and parameters

adds test for tree search functions. Adds doctstring for get_siblings…

e6c9662

… and is_stochastic functions. Corrects syntax error when initializing history_by_cond in constructor

merged from several branches and resolve conflict

e742004

meatballs approved these changes May 2, 2017

View reviewed changes

drvinceknight approved these changes May 2, 2017

View reviewed changes

meatballs added the ready-to-merge label May 3, 2017

marcharper approved these changes May 5, 2017

View reviewed changes

marcharper merged commit 4891d18 into Axelrod-Python:master May 5, 2017

		return MoveGen((self.history[-1],opponent.history[-1]),self.Pi,depth_search_tree=self.tree_depth)


		# Policy as defined in the article, i.e. a set of (last_move,p) where p is

Dbs #976

Dbs #976

Conversation

edouardArgenson commented Apr 16, 2017

marcharper commented Apr 17, 2017

marcharper left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edouardArgenson Apr 17, 2017 • edited Loading

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

edouardArgenson Apr 17, 2017 • edited Loading

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 Apr 17, 2017

edouardArgenson commented Apr 17, 2017

edouardArgenson commented Apr 20, 2017

edouardArgenson commented Apr 20, 2017

drvinceknight commented Apr 20, 2017

drvinceknight commented Apr 20, 2017

edouardArgenson commented Apr 20, 2017 • edited Loading

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

edouardArgenson Apr 27, 2017 • edited Loading

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

Choose a reason for hiding this comment

edouardArgenson Apr 27, 2017 • edited Loading

Choose a reason for hiding this comment

edouardArgenson Apr 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcharper commented Apr 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edouardArgenson commented Apr 27, 2017

drvinceknight commented May 1, 2017

edouardArgenson commented May 1, 2017

edouardArgenson Apr 17, 2017 •

edited

Loading

edouardArgenson Apr 17, 2017 •

edited

Loading

edouardArgenson commented Apr 20, 2017 •

edited

Loading

edouardArgenson Apr 27, 2017 •

edited

Loading

edouardArgenson Apr 27, 2017 •

edited

Loading

edouardArgenson Apr 27, 2017 •

edited

Loading