Codecov Report

Merging #367 (29e7cae) into master (b5c3dda) will decrease coverage by 0.01%.
The diff coverage is 93.54%.

@@            Coverage Diff             @@
##           master     #367      +/-   ##
==========================================
- Coverage   94.55%   94.53%   -0.02%     
==========================================
  Files          54       55       +1     
  Lines        3505     3567      +62     
==========================================
+ Hits         3314     3372      +58     
- Misses        191      195       +4     

Flags with carried forward coverage won't be shown. Click here to find out more.

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b5c3dda...29e7cae. Read the comment docs.

FYI, I tried a variant of CRR where I replaced the DQN critic with a QRDQN (num_quantiles=200). It was slightly better but not much: For Breakout, after 40 iterations, I got 161.4 for QRCRR vs 137.2 for CRR; for Pong, after 40 iterations, I got 18.5 for QRCRR vs 16.8 for CRR.

Have you ever considered inheriting from QRDQN or DiscreteCQL instead of PG? Because the only reused part is forward and it is really simple (just a few line, just discrete, no other cases in PGPolicy) -- and maybe that would be more concise?

Yes, I have. But after some thought, I chose to inherit from PGPolicy to emphasize the fact that CRR is a policy gradient model, i.e., the critic, be it DQN, QRDQN or else, is only a by-product of the training process and does NOT contribute to the evaluation of the policy. I also thought about multiple inheritance, i.e., let CRR inherits from both PGPolicy and DQN or QRDQN or DiscreteCQL but I realized that it would not make the implementation simpler. For example, because none of them has a self.actor_old model, I still need to override the def sync_weight() method in order to synchronize the target actor model.

The only place I can think of improving is to refactor the def learn() method of the parent class in order to share some code. But even if I do that, that's only going to save me <10 lines of code at the expense of fragmenting the logic of def learn() function.

Anyway that's just my opinion. I can certainly change it if you insist.