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.

Sign up for GitHub

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

Jump to bottom

(Idea) feature: init hidden torch #243

Merged
merged 1 commit into from
Jan 25, 2022
Merged

(Idea) feature: init hidden torch #243

merged 1 commit into from
Jan 25, 2022

Conversation

YuriCat
Copy link
Contributor

@YuriCat YuriCat commented Jan 23, 2022 •
edited
Loading

Currently, users have to define init_hidden() method like this:

def init_hidden(self, batch_size)
    if batch_size is None:
        return numpy-array
    else:
        return torch-tensor

, while this writing rule has little advantage except small speedup.

ikki407
ikki407 reviewed Jan 24, 2022
View reviewed changes
handyrl/envs/geister.py
@@ -150,7 +144,7 @@ def __init__(self):
self.head_v = ScalarHead((filters * 2, 6, 6), 1, 1)
self.head_r = ScalarHead((filters * 2, 6, 6), 1, 1)

def init_hidden(self, batch_size=None):
def init_hidden(self, batch_size=[]):
Copy link
Member

Choose a reason for hiding this comment

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

IMHO, I prefer batch_size=None like this. What do you think?
Also, https://github.com/DeNA/HandyRL/blob/master/handyrl/evaluation.py#L79

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed this to show that we don't need if branch, but since this is a user-defined location, None is also fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it needs to be the default argument.

Copy link
Member

Choose a reason for hiding this comment

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

👍

@ikki407
Copy link
Member

ikki407 commented Jan 24, 2022

LGTM except for a comment above.

@ikki407 ikki407 mentioned this pull request Jan 24, 2022
Merged
@ikki407 ikki407 merged commit fd04681 into DeNA:develop Jan 25, 2022
@YuriCat YuriCat deleted the feature/init_hidden_torch branch February 11, 2022 17:56
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ikki407 ikki407 ikki407 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@YuriCat @ikki407