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
403 stars 59 forks source link

Torch Wrappers to clean out numpy #294

Closed Sharad24 closed 4 years ago

Sharad24 commented 4 years ago

Need to update ReplayBuffer, documentation and typing.

lgtm-com[bot] commented 4 years ago

This pull request introduces 5 alerts when merging c55e8341f9b77e4709f4758cf30372c6ca60b710 into dc2d58bdade1c5239ccc0925aeda78d1dc59e972 - view on LGTM.com

new alerts:

Sharad24 commented 4 years ago

I reckon it might be better to shift all env wrappers to torch now. And keeping only .numpy() for base env (i.e., just before a function which requires a numpy array e.g. step())

Unless there's a limitation to torch not working with a particular wrapper that is. @sampreet-arthi Any comments?

sampreet-arthi commented 4 years ago

I reckon it might be better to shift all env wrappers to torch now. And keeping only .numpy() for base env (i.e., just before a function which requires a numpy array e.g. step())

Would be a good idea imo. This might help increase speed a lot. I don't think there's any environment which will not work with any env.

lgtm-com[bot] commented 4 years ago

This pull request introduces 22 alerts when merging 8b92fb82b72e9c9f27dded95355432faf952b4b3 into dc2d58bdade1c5239ccc0925aeda78d1dc59e972 - view on LGTM.com

new alerts:

Sharad24 commented 4 years ago

Will merge this today. Most likely done here, will take a pass once again though.

lgtm-com[bot] commented 4 years ago

This pull request introduces 24 alerts when merging ab09539e2f674576d2a2900febe76f9d4b2c0b71 into e93cdcc2527b94f3815c25aa0fe9e11a0903822b - view on LGTM.com

new alerts:

lgtm-com[bot] commented 4 years ago

This pull request introduces 5 alerts when merging d7730f87be880f9cd0921edc8e188b72c78e72de into c20464fc065cf15b7da451f15cfb157e984dcd09 - view on LGTM.com

new alerts:

codecov[bot] commented 4 years ago

Codecov Report

Merging #294 into master will increase coverage by 1.24%. The diff coverage is 97.59%.

@@            Coverage Diff             @@
##           master     #294      +/-   ##
==========================================
+ Coverage   89.63%   90.87%   +1.24%     
==========================================
  Files          87       88       +1     
  Lines        3752     3705      -47     
==========================================
+ Hits         3363     3367       +4     
+ Misses        389      338      -51     
Impacted Files Coverage Δ
genrl/agents/deep/td3/td3.py 92.72% <ø> (-0.13%) :arrow_down:
genrl/agents/deep/vpg/vpg.py 94.33% <ø> (-0.21%) :arrow_down:
genrl/trainers/onpolicy.py 92.00% <ø> (ø)
genrl/utils/utils.py 94.25% <77.77%> (-2.09%) :arrow_down:
genrl/core/rollout_storage.py 91.53% <94.44%> (+0.06%) :arrow_up:
genrl/environments/torch.py 95.00% <95.00%> (ø)
...nrl/agents/bandits/contextual/common/base_model.py 100.00% <100.00%> (ø)
genrl/agents/bandits/contextual/common/bayesian.py 92.30% <100.00%> (ø)
genrl/agents/bandits/contextual/common/neural.py 95.23% <100.00%> (ø)
genrl/agents/deep/a2c/a2c.py 93.24% <100.00%> (-0.18%) :arrow_down:
... and 23 more
Sharad24 commented 4 years ago

Checked all agents. Training properly. Everything working smoothly otherwise.

lgtm-com[bot] commented 4 years ago

This pull request introduces 15 alerts when merging 8bae9a8dd4b3518cecc629a24ccddd59cd770f02 into 52860956b6970620620ee37c9c94f4d65abb1d84 - view on LGTM.com

new alerts:

lgtm-com[bot] commented 4 years ago

This pull request introduces 14 alerts when merging 1b9af91b65c3b3c384ad04a4cfda4f151569ef91 into 52860956b6970620620ee37c9c94f4d65abb1d84 - view on LGTM.com

new alerts:

lgtm-com[bot] commented 4 years ago

This pull request introduces 14 alerts when merging 3735077f81ee9eca02b21d6a0b8e99accb044118 into 52860956b6970620620ee37c9c94f4d65abb1d84 - view on LGTM.com

new alerts:

lgtm-com[bot] commented 4 years ago

This pull request introduces 13 alerts when merging 4bdbf99d5d8efedb1fca5b813bfd1bfdc16d7901 into 52860956b6970620620ee37c9c94f4d65abb1d84 - view on LGTM.com

new alerts:

lgtm-com[bot] commented 4 years ago

This pull request introduces 13 alerts when merging 9e7cfe879e82a42fb7a8ba69d51130b9638b61e1 into 52860956b6970620620ee37c9c94f4d65abb1d84 - view on LGTM.com

new alerts:

sampreet-arthi commented 4 years ago

Just remembered that the reason we stuck with isort 4.3.20 was because we could avoid the LGTM alerts :P

Sharad24 commented 4 years ago

Yeah this is getting really annoying. LGTM config file does not seem to do the job, # lgtm[query-id] also is not working.

noqa is the last attempt, otherwise switching back to 4.3.2

lgtm-com[bot] commented 4 years ago

This pull request introduces 12 alerts when merging df410a8a51b3f9afef1440f63bc3ceda42384b55 into 52860956b6970620620ee37c9c94f4d65abb1d84 - view on LGTM.com

new alerts:

lgtm-com[bot] commented 4 years ago

This pull request introduces 4 alerts when merging 96d79bd2b953b047078c92c0d28bdf3242b76e49 into 52860956b6970620620ee37c9c94f4d65abb1d84 - view on LGTM.com

new alerts: