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

GaussianLSTMPolicy with model #677

Merged
merged 4 commits into from
Jun 1, 2019
Merged

GaussianLSTMPolicy with model #677

merged 4 commits into from
Jun 1, 2019

Conversation

ahtsan
Copy link
Contributor

@ahtsan ahtsan commented May 23, 2019

Added GaussianLSTMModel and GaussianLSTMPolicyWithModel.

Added test for PPO with GaussianLSTMPolicyWithModel.

Apart from testing functionality of GaussianLSTMPolicyWithModel
in test_gaussian_lstm_policy_with_model.py, transitions from the
old policy (GaussianLSTMPolicy) to the new policy
(GaussianLSTMPolicyWithModel) are also tested in
test_gaussian_lstm_policy_with_model_transit.py, to make sure
they have the same API.

@ahtsan ahtsan requested a review from a team as a code owner May 23, 2019 04:33
@codecov
Copy link

codecov bot commented May 23, 2019

Codecov Report

Merging #677 into master will increase coverage by 0.53%.
The diff coverage is 98.7%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #677      +/-   ##
==========================================
+ Coverage   64.08%   64.62%   +0.53%     
==========================================
  Files         159      161       +2     
  Lines        9770     9924     +154     
  Branches     1293     1303      +10     
==========================================
+ Hits         6261     6413     +152     
- Misses       3182     3183       +1     
- Partials      327      328       +1
Impacted Files Coverage Δ
src/garage/tf/policies/gaussian_lstm_policy.py 78.83% <ø> (ø) ⬆️
src/garage/tf/models/gaussian_lstm_model.py 100% <100%> (ø)
...age/tf/policies/gaussian_lstm_policy_with_model.py 97.7% <97.7%> (ø)

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 95f0295...57206f1. Read the comment docs.

@ryanjulian ryanjulian changed the title Gaussian LSTM Policy with model GaussianLSTMPolicy with model May 23, 2019
@@ -0,0 +1,295 @@
"""GaussianLSTMPolicy with GaussianLSTMModel."""
from akro.tf import Box
Copy link
Member

Choose a reason for hiding this comment

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

import akro.tf

Copy link
Member

@zhanpenghe zhanpenghe left a comment

Choose a reason for hiding this comment

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

I only have minor comments. Is the *LSTMModel always be single layer recurrent unit now?

src/garage/tf/models/gaussian_lstm_model.py Show resolved Hide resolved

Returns:
action (numpy.ndarray): Predicted action.
agent_info (dict[numpy.ndarray]): Mean and log std of the
Copy link
Member

Choose a reason for hiding this comment

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


Returns:
actions (numpy.ndarray): Predicted actions.
agent_infos (dict[numpy.ndarray]): Mean and log std of the
Copy link
Member

Choose a reason for hiding this comment

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


Returns:
action (numpy.ndarray): Predicted action.
agent_info (dict[numpy.ndarray]): Mean and log std of the
Copy link
Member

Choose a reason for hiding this comment

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

@zhanpenghe
Copy link
Member

Can you please make it multi layer recurrent? Single layer sometimes does not work well for hard problem..

@ahtsan
Copy link
Contributor Author

ahtsan commented May 30, 2019

@zhanpenghe I think another PR will be more appropriate.

Copy link
Member

@zhanpenghe zhanpenghe left a comment

Choose a reason for hiding this comment

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

Sure

@zhanpenghe
Copy link
Member

File an issue then.

@ahtsan ahtsan merged commit e0da813 into master Jun 1, 2019
@ahtsan ahtsan deleted the gaussian_lstm branch June 1, 2019 00:41
cheng-kevin pushed a commit that referenced this pull request Jun 3, 2019
This PR refactored GaussianLSTMPolicy with garage.tf.models.Model.
It added two classes: GaussianLSTMModel and GaussianLSTMPolicyWithModel.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants