SforAiDl / genrl

A PyTorch reinforcement learning library for generalizable and reproducible algorithm implementations with an aim to improve accessibility in RL
https://genrl.readthedocs.io
MIT License
404 stars 59 forks source link

Seperate compute_returns_and_advantage #328

Closed hades-rp2010 closed 3 years ago

hades-rp2010 commented 3 years ago

Wrt #320

Preferably merge after #307 (Merging 307 will remove the conflicts in this PR)

hades-rp2010 commented 3 years ago

Wrt #320

Preferably merge after #307 (Merging 307 will remove the conflicts in this PR)

My bad, merge conflicts are resolved

codecov[bot] commented 3 years ago

Codecov Report

Merging #328 into master will decrease coverage by 0.95%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #328      +/-   ##
==========================================
- Coverage   91.23%   90.27%   -0.96%     
==========================================
  Files          88       89       +1     
  Lines        3777     3765      -12     
==========================================
- Hits         3446     3399      -47     
- Misses        331      366      +35     
Impacted Files Coverage Δ
genrl/agents/deep/base/offpolicy.py 97.40% <ø> (ø)
genrl/core/rollout_storage.py 89.62% <ø> (-1.92%) :arrow_down:
genrl/agents/deep/a2c/a2c.py 93.58% <100.00%> (ø)
genrl/agents/deep/base/base.py 93.75% <100.00%> (ø)
genrl/agents/deep/ddpg/ddpg.py 92.85% <100.00%> (ø)
genrl/agents/deep/ppo1/ppo1.py 100.00% <100.00%> (ø)
genrl/agents/deep/sac/sac.py 93.33% <100.00%> (ø)
genrl/agents/deep/td3/td3.py 93.10% <100.00%> (ø)
genrl/agents/deep/vpg/vpg.py 94.33% <100.00%> (ø)
genrl/core/actor_critic.py 97.94% <100.00%> (ø)
... and 17 more
hades-rp2010 commented 3 years ago

Should this function even be part of rollout_storage now?

I think it makes more sense to shift this function inside the OnPolicyAgent now

lgtm-com[bot] commented 3 years ago

This pull request introduces 2 alerts when merging 6453ee010a6c986fef0bb3d37aeafdb58dd2e6f4 into 0fe41807d67ea504e0b57dc429402a3a99d2c253 - view on LGTM.com

new alerts:

hades-rp2010 commented 3 years ago

@Sharad24 If no other changes to this are reqd, this could be merged as well

Sharad24 commented 3 years ago

Can you sync this with master? Will merge it after that.