CEM #373
CEM #373
Conversation
Codecov Report
@@ Coverage Diff @@
## master #373 +/- ##
==========================================
- Coverage 91.28% 91.25% -0.04%
==========================================
Files 90 92 +2
Lines 3809 3910 +101
==========================================
+ Hits 3477 3568 +91
- Misses 332 342 +10
|
|
This pull request introduces 3 alerts when merging a90e8d0 into 52b0b4c - view on LGTM.com new alerts:
|
|
This pull request introduces 4 alerts when merging 3b2067d into 25eb018 - view on LGTM.com new alerts:
|
genrl/agents/modelbased/base.py
Outdated
| raise NotImplementedError | ||
|
|
||
|
|
||
| class ModelBasedAgent(ABC): |
There was a problem hiding this comment.
Can this inherit from the genrl/deep BaseAgent?
There was a problem hiding this comment.
It can, and I think thats a better option (for now at least)
| # No need for this here | ||
| pass | ||
|
|
||
| def collect_rollouts(self, state: torch.Tensor): |
There was a problem hiding this comment.
Looks pretty similar to the OnPolicyAgent method. Shouldn't this return values and dones though? Not sure if this is a consequence of the algo.
genrl/agents/modelbased/cem/cem.py
Outdated
| for i, done in enumerate(dones): | ||
| if done or timestep == self.rollout_size - 1: | ||
| self.rewards.append(self.env.episode_reward[i].detach().clone()) | ||
| # self.env.reset_single_env(i) |
There was a problem hiding this comment.
Why is this commented out? This is necessary to reset environments immediately as they are set to done. (Not a good practice to do env.step() if the env is already returning done = True)
There was a problem hiding this comment.
Since I am breaking the loop of actions if a env.step() returns done=True, and every plan session (the plan function) starts with env.reset(), I think this is redundant here, hence its commented out
|
|
||
|
|
||
| def test_CEM(): | ||
| env = VectorEnv("CartPole-v0", 1) |
There was a problem hiding this comment.
Why set it to 1? It does work with multiple envs right?
|
Also, forgot to mention the docs. The CEM agent code didn't have docstrings afair. |
| from genrl.trainers import OnPolicyTrainer | ||
|
|
||
|
|
||
| def test_CEM(): |
There was a problem hiding this comment.
Also please make this a class so the tests are easier to find/understand
Yeah, I'll get that done too |
|
This pull request introduces 2 alerts when merging f5a189d into 25eb018 - view on LGTM.com new alerts:
|
|
This pull request introduces 2 alerts when merging 4b11c16 into 25eb018 - view on LGTM.com new alerts:
|
Wrt #363