Answers.Body: <p>Yeah, it seems like it's a wrong implementation.
  vals_ref_v is a matrix of 1 row, and 128 columns.
  value_v.detach() is a matrix of 128 row </p>
  
Answers.ContentLicense: CC BY-SA 4.0
Answers.CreationDate: 2020-05-16T01:04:00.050Z
Answers.Text: https://github.com/PacktPublishing/Deep-Reinforcement-Learning-Hands-On/issues/74
Answers.UserDisplayName: jgauth
Answers.ContentLicense: CC BY-SA 4.0
Answers.CreationDate: 2020-05-16T00:50:41.687Z
Answers.LastActivityDate: 2020-05-16T00:50:41.687Z
Answers.UserDisplayName: C Lu
Answers.Body: <p>I changed the line <code>adv_v = vals_ref_v - value_v.detach()</code> to <code>adv_v = vals_ref_v - value_v.squeeze(-1).detach()</code>. It seems the convergence is much faster. According to the A2C algorithm, it is just logic to apply <span class="math-container">$Q(a, s) - V(s)$</span>, where <span class="math-container">$Q(a, s)$</span> and <span class="math-container">$V(s)$</span> with the same shape.  </p>
  
  <p>The call to <code>detach()</code> is important here as we don't want to propagate the PG into our value approximation head.</p>
  
Answers.ContentLicense: CC BY-SA 4.0
Answers.CreationDate: 2020-05-15T20:27:37.550Z
Answers.LastActivityDate: 2020-05-15T20:27:37.550Z
Answers.UserDisplayName: jgauth
Body: <p>Here is the <a href="https://github.com/PacktPublishing/Deep-Reinforcement-Learning-Hands-On/blob/master/Chapter10/02_pong_a2c.py" rel="nofollow noreferrer">code</a> written by Maxim Lapan. I am reading his book (<em>Deep Reinforcement Learning Hands-on</em>). I have seen a line in his code which is really weird. In the accumulation of the policy gradient <span class="math-container">$$\partial \theta_{\pi} \gets \partial \theta_{\pi} + \nabla_{\theta}\log\pi_{\theta} (a_i | s_i) (R - V_{\theta}(s_i))$$</span> we have to compute the advantage <span class="math-container">$R - V_{\theta}(s_i)$</span>. In line 138, maxim uses <code>adv_v = vals_ref_v - value_v.detach()</code>. Visually, it looks fine, but look at the shape of each term. </p>
  
  <pre><code>ipdb&gt; adv_v.shape                                                                                                                            
  torch.Size([128, 128])
  
  ipdb&gt; vals_ref_v.shape                                                                                                                       
  torch.Size([128])
  
  ipdb&gt; values_v.detach().shape                                                                                                                
  torch.Size([128, 1]) 
  </code></pre>
  
  <p>In a much simpler code, it is equivalent to </p>
  
  <pre><code>In [1]: import torch                                                            
  
  In [2]: t1 = torch.tensor([1, 2, 3])                                            
  
  In [3]: t2 = torch.tensor([[4], [5], [6]])                                      
  
  In [4]: t1 - t2                                                                 
  Out[4]: 
  tensor([[-3, -2, -1],
          [-4, -3, -2],
          [-5, -4, -3]])
  
  In [5]: t1 - t2.detach()                                                        
  Out[5]: 
  tensor([[-3, -2, -1],
          [-4, -3, -2],
          [-5, -4, -3]])
  </code></pre>
  
  <p>I have trained the agent with his code and it works perfectly fine. I am very confused why it is good practice and what it is doing. Could someone enlighten me on the line <code>adv_v = vals_ref_v - value_v.detach()</code>? For me, the right thing to do was <code>adv_v = vals_ref_v - value_v.squeeze(-1)</code>.</p>
  
  <p>Here is the full algorithm used in his book : </p>
  
  <p><strong>UPDATE</strong> </p>
  
  <p><a href="https://i.sstatic.net/aNOfJ.png" rel="nofollow noreferrer"><img src="https://i.sstatic.net/aNOfJ.png" alt="enter image description here"></a></p>
  
  <p>As you can see by the image, it is converging even though <code>adv_v = vals_ref_v - value_v.detach()</code> looks wrongly implemented. It is not done yet, but I will update the question later.</p>
  
Comments.ContentLicense: CC BY-SA 4.0
Comments.CreationDate: 2020-05-15T07:04:52.317Z
Comments.Text: Could you clarify, you trained the agent with his code or with your modified code? If both, did you notice any difference in behaviour?
Comments.UserDisplayName: dan888
Comments.ContentLicense: CC BY-SA 4.0
Comments.CreationDate: 2020-05-15T11:20:33.167Z
Comments.Text: @dan888 With his code. I just cloned the repo.
Comments.UserDisplayName: jgauth
Comments.ContentLicense: CC BY-SA 4.0
Comments.CreationDate: 2020-05-15T13:04:32.703Z
Comments.Text: That's interesting. I would say that what you are doing would make more sense as well. I noticed you created an issue on github, I'm curious to see what comes out of that.
Comments.UserDisplayName: dan888
Comments.ContentLicense: CC BY-SA 4.0
Comments.CreationDate: 2020-05-15T13:22:53.077Z
Comments.Text: He used the same line many times in his book, but I can't say why it is working and what it is doing exactly. I am going to update the question in few hours to add a graph of what the mean rewards look like. In his book, maxim said *The call to `value_v.detach()` is important, as we don't want to propagate the PG into our value approximation head*. That didn't help me, but maybe it will help someone.
Comments.UserDisplayName: jgauth
Tags: <python><policy-gradients><pytorch><actor-critic-methods>
Title: Advantage computed the wrong way?
UserDisplayName: jgauth
ThreadText: # Advantage computed the wrong way?
  
  (Asked by: username_0 on 2020-05-14)
  
  Here is the code written by Maxim Lapan. I am reading his book (Deep Reinforcement Learning Hands-on). I have seen a line in his code which is really weird. In the accumulation of the policy gradient $$\partial \theta_{\pi} \gets \partial \theta_{\pi} + \nabla_{\theta}\log\pi_{\theta} (a_i | s_i) (R - V_{\theta}(s_i))$$ we have to compute the advantage $R - V_{\theta}(s_i)$. In line 138, maxim uses adv_v = vals_ref_v - value_v.detach(). Visually, it looks fine, but look at the shape of each term. 
  ipdb> adv_v.shape                                                                                                                            
  torch.Size([128, 128])
  
  ipdb> vals_ref_v.shape                                                                                                                       
  torch.Size([128])
  
  ipdb> values_v.detach().shape                                                                                                                
  torch.Size([128, 1]) 
  
  In a much simpler code, it is equivalent to 
  In [1]: import torch                                                            
  
  In [2]: t1 = torch.tensor([1, 2, 3])                                            
  
  In [3]: t2 = torch.tensor([[4], [5], [6]])                                      
  
  In [4]: t1 - t2                                                                 
  Out[4]: 
  tensor([[-3, -2, -1],
          [-4, -3, -2],
          [-5, -4, -3]])
  
  In [5]: t1 - t2.detach()                                                        
  Out[5]: 
  tensor([[-3, -2, -1],
          [-4, -3, -2],
          [-5, -4, -3]])
  
  I have trained the agent with his code and it works perfectly fine. I am very confused why it is good practice and what it is doing. Could someone enlighten me on the line adv_v = vals_ref_v - value_v.detach()? For me, the right thing to do was adv_v = vals_ref_v - value_v.squeeze(-1).
  Here is the full algorithm used in his book : 
  UPDATE 
  
  As you can see by the image, it is converging even though adv_v = vals_ref_v - value_v.detach() looks wrongly implemented. It is not done yet, but I will update the question later.
  
  **Tags:** python, policy-gradients, pytorch, actor-critic-methods
  > **Comments:**
  > **username_1**: Could you clarify, you trained the agent with his code or with your modified code? If both, did you notice any difference in behaviour?  
  > **username_0**: @dan888 With his code. I just cloned the repo.  
  > **username_1**: That's interesting. I would say that what you are doing would make more sense as well. I noticed you created an issue on github, I'm curious to see what comes out of that.  
  > **username_0**: He used the same line many times in his book, but I can't say why it is working and what it is doing exactly. I am going to update the question in few hours to add a graph of what the mean rewards look like. In his book, maxim said *The call to `value_v.detach()` is important, as we don't want to propagate the PG into our value approximation head*. That didn't help me, but maybe it will help someone.  
  
  ---
  
  ## 2 answers
  
  ### Answer by username_2 on 2020-05-16. Score: 2
  
  Yeah, it seems like it's a wrong implementation.
  vals\_ref\_v is a matrix of 1 row, and 128 columns.
  value\_v.detach() is a matrix of 128 row
  
  > **Comments:**
  > **username_0**: https://github.com/PacktPublishing/Deep-Reinforcement-Learning-Hands-On/issues/74  
  
  ---
  
  ### Answer by username_0 on 2020-05-15. Score: 1
  
  I changed the line `adv_v = vals_ref_v - value_v.detach()` to `adv_v = vals_ref_v - value_v.squeeze(-1).detach()`. It seems the convergence is much faster. According to the A2C algorithm, it is just logic to apply $Q(a, s) - V(s)$, where $Q(a, s)$ and $V(s)$ with the same shape.
  
  The call to `detach()` is important here as we don't want to propagate the PG into our value approximation head.
  
  
  ---
  
  