Closed
Conversation
This is meant for an RNN and hidden state is the standard terminology, plus `state` generally has a very different meaning in an RL library.
`self.sess` isn't defined, and all the other methods call `self._sess`
The only real time this file is modified is to add/rm from one of those lists, so we may as well make the diff easy by putting every element on its own line.
`tf.{add,square,multiply,abs}` all have overloaded operators.
`0.5 * foo` is easier to read in almost all cases as `foo / 2`.
Rollout should have .items() implemented, which makes this easier to read.
It's a short function and may as well be made even easier to read.
`exp_vals` sounds like it has to do with the exponential function. Since it's only used in this one spot, may as well be explicit since it doesn't really cost much brevity.
|
Test FAILed. |
Contributor
|
@alok, while cleaning up code as you add features is always appreciated, there is little value in changing things for the sake of changing them. I'm closing this since most of the changes seem superficial and a matter of opinion as to whether they are better or worse. I am planning on linting all of rllib at some point when convenient though -- so no need to worry about that ;) |
Contributor
Author
|
Ok, though I'm pretty sure the self.sess should be self._sess since self.sess isn't defined.
|
Contributor
|
Can u open a PR with just that fix? I guess all the subclasses must set
self.sess so it passes tests.
…On Sat, Jun 9, 2018, 10:14 AM Alok Singh ***@***.***> wrote:
Ok, though I'm pretty sure the self.sess should be self._sess
On Sat, Jun 9, 2018 at 9:25 AM Eric Liang ***@***.***>
wrote:
> Closed #2224 <#2224>.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#2224 (comment)>, or mute
> the thread
> <
https://github.com/notifications/unsubscribe-auth/AH8KTHlb6Zipe2EhJSrr4n-ZtQVltaPFks5t6_bcgaJpZM4UhQPW
>
> .
>
--
- Alok
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#2224 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAA6SvByqRdBsOTRh2h3zKcbLnK-d9PQks5t7AJhgaJpZM4UhQPW>
.
|
Contributor
Author
|
If every subclass ends up setting self.sess, is it worth it to have both |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What do these changes do?
Small followup to #2149.
Mostly cleans up random little things.
@ericl please check this out. I broke it into separate commits to make reverting
each part easier.
Related issue number
#2149.