Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RL models clean up #112

Conversation

djbyrne
Copy link
Contributor

@djbyrne djbyrne commented Jul 12, 2020

Before submitting

  • Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
  • Did you read the contributor guideline, Pull Request section?
  • Did you make sure to update the docs?
  • Did you write any new necessary tests?

What does this PR do?

Refactors RL models to use train_batch structure as seen in #107

general clean up of docstrings and methods

PR review

Did you have fun?

👍

@pep8speaks
Copy link

pep8speaks commented Jul 12, 2020

Hello @djbyrne! Thanks for updating this PR.

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-07-14 15:48:24 UTC

@mergify mergify bot requested a review from Borda July 12, 2020 10:00
@codecov
Copy link

codecov bot commented Jul 12, 2020

Codecov Report

Merging #112 into master will increase coverage by 0.00%.
The diff coverage is 86.48%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #112   +/-   ##
=======================================
  Coverage   91.91%   91.92%           
=======================================
  Files          77       78    +1     
  Lines        3944     4010   +66     
=======================================
+ Hits         3625     3686   +61     
- Misses        319      324    +5     
Flag Coverage Δ
#unittests 91.92% <86.48%> (+<0.01%) ⬆️
Impacted Files Coverage Δ
pl_bolts/models/rl/double_dqn_model.py 94.44% <ø> (+20.25%) ⬆️
pl_bolts/models/rl/n_step_dqn_model.py 100.00% <ø> (ø)
pl_bolts/models/rl/noisy_dqn_model.py 95.83% <ø> (+18.05%) ⬆️
pl_bolts/models/rl/per_dqn_model.py 57.14% <9.09%> (-23.81%) ⬇️
pl_bolts/models/rl/dqn_model.py 82.88% <75.00%> (+0.35%) ⬆️
pl_bolts/datamodules/experience_source.py 97.72% <97.72%> (ø)
pl_bolts/datamodules/__init__.py 100.00% <100.00%> (ø)
pl_bolts/models/rl/common/experience.py 97.08% <100.00%> (+0.14%) ⬆️
pl_bolts/models/rl/reinforce_model.py 97.43% <100.00%> (+0.02%) ⬆️
...l_bolts/models/rl/vanilla_policy_gradient_model.py 96.55% <100.00%> (-1.16%) ⬇️
... and 11 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 024b574...79e7e5c. Read the comment docs.

@Borda Borda changed the title Enhancement/rl models clean up RL models clean up Jul 12, 2020
@Borda Borda added the enhancement New feature or request label Jul 12, 2020
Comment on lines +93 to +94
self.episode_reward = 0
self.episode_steps = 0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shall this be rater at the beginning rather than the end?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Im not sure I understand

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that you reset the episode_steps and the other when it is done... so shall it be more logical to reset it before you start rather?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see. The done is a local variable that is retrieved after taking a step on line 72, so the done check must come after that, so I think it makes more sense to do it at the end

@mergify
Copy link
Contributor

mergify bot commented Jul 29, 2020

This pull request is now in conflict... :(

@Borda Borda changed the base branch from master to master_RL September 8, 2020 21:22
@Borda Borda closed this Oct 7, 2020
@Borda Borda added this to Done in Reinforcement Learning Nov 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

None yet

4 participants