-
Notifications
You must be signed in to change notification settings - Fork 310
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
Rename self.evaluate for off-policy algos #1139
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comment about deleting this bool altogether. It would make these classes less likely to suffer a logic error in the future, because storing conditionals is dangerous (it makes your class a state machine).
Can you make the members in these classes _private
while you're here?
@@ -65,7 +65,7 @@ def __init__(self, | |||
self.min_buffer_size = min_buffer_size | |||
self.rollout_batch_size = rollout_batch_size | |||
self.reward_scale = reward_scale | |||
self.evaluate = False | |||
self._first_buffer_size_steps_done = False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This tracks whether the replay buffer is at least min_buffer_size
right? So why not just test that directly?
self.replay_buffer.n_transitions_stored >= self.min_buffer_size
Perhaps the original author was concerned about having such a long conditional everywhere? If so, you can add a helper property:
@property
def _buffer_prefilled(self):
return self.replay_buffer.n_transitions_stored >= self.min_buffer_size
@@ -65,7 +65,7 @@ def __init__(self, | |||
self.min_buffer_size = min_buffer_size | |||
self.rollout_batch_size = rollout_batch_size | |||
self.reward_scale = reward_scale | |||
self.evaluate = False | |||
self._first_buffer_size_steps_done = False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This tracks whether the replay buffer is at least min_buffer_size
right? So why not just test that directly?
self.replay_buffer.n_transitions_stored >= self.min_buffer_size
Perhaps the original author was concerned about having such a long conditional everywhere? If so, you can add a helper property:
@property
def _buffer_prefilled(self):
return self.replay_buffer.n_transitions_stored >= self.min_buffer_size
66fc298
to
7d6bf26
Compare
Codecov Report
@@ Coverage Diff @@
## master #1139 +/- ##
==========================================
+ Coverage 87.98% 87.99% +0.01%
==========================================
Files 184 184
Lines 8780 8771 -9
Branches 1108 1108
==========================================
- Hits 7725 7718 -7
+ Misses 856 854 -2
Partials 199 199
Continue to review full report at Codecov.
|
@krzentner is this PR still relevant? |
- also does duplicate logging removal
7d6bf26
to
ccf70c5
Compare
evaluate_performance()
being introduced.