Skip to content

Fix policy entropy #368

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
Merged
CatherineSue merged 1 commit into from
Oct 30, 2018
Merged

Fix policy entropy #368

CatherineSue merged 1 commit into from
Oct 30, 2018

Conversation

@CatherineSue
Copy link
Member

@CatherineSue CatherineSue commented Oct 29, 2018 •
edited
Loading

This commit fixes policy entropy. Currently it uses the following three
options:
• maximum entropy (augmented reward)
• with a logli estimator (-logli(action|policy)
• with the gradient turned off (tf.stop_gradient(entropy))

See: #281

@CatherineSue CatherineSue requested a review from a team as a code owner October 29, 2018 17:18
@codecov
Copy link

codecov bot commented Oct 29, 2018 •
edited
Loading

Codecov Report

Merging #368 into master will increase coverage by 0.1%.
The diff coverage is 91.66%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #368     +/-   ##
=========================================
+ Coverage   64.62%   64.72%   +0.1%     
=========================================
  Files         212      212             
  Lines       13977    13985      +8     
=========================================
+ Hits         9032     9052     +20     
+ Misses       4945     4933     -12
Impacted Files Coverage Δ
garage/tf/algos/npo.py 95.02% <91.66%> (-0.32%) ⬇️
garage/tf/distributions/diagonal_gaussian.py 66.66% <0%> (-5%) ⬇️
garage/envs/mujoco/gather/gather_env.py 57.57% <0%> (+0.67%) ⬆️
.../theano/optimizers/conjugate_gradient_optimizer.py 74.05% <0%> (+1.26%) ⬆️
garage/envs/mujoco/maze/maze_env.py 82.21% <0%> (+1.44%) ⬆️
garage/theano/optimizers/first_order_optimizer.py 85.5% <0%> (+1.44%) ⬆️
garage/tf/distributions/categorical.py 66.66% <0%> (+1.51%) ⬆️
garage/tf/distributions/recurrent_categorical.py 79.16% <0%> (+4.16%) ⬆️
garage/envs/mujoco/point_env.py 77.77% <0%> (+11.11%) ⬆️

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 e89ce7a...3589a48. Read the comment docs.

ryanjulian
ryanjulian reviewed Oct 29, 2018
View reviewed changes
Copy link
Member

@ryanjulian ryanjulian left a comment

Choose a reason for hiding this comment

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

Did this fix the behavior where the entropy term has no effect?

garage/tf/algos/npo.py Outdated
policy_entropy = tf.reduce_mean(policy_entropy * i.valid_var)
policy_entropy = policy_entropy * i.valid_var

if self._stop_entropy_gradients:
Copy link
Member

@ryanjulian ryanjulian Oct 29, 2018

Choose a reason for hiding this comment

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

stop_entropy_gradient

garage/tf/algos/npo.py Outdated
policy=None,
policy_ent_coeff=1e-2,
use_softplus_entropy=True,
log_estimate_entropy=True,
Copy link
Member

@ryanjulian ryanjulian Oct 29, 2018

Choose a reason for hiding this comment

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

use_logli_entropy

garage/tf/algos/npo.py
name="NPO",
policy=None,
policy_ent_coeff=1e-2,
use_softplus_entropy=True,
Copy link
Member

@ryanjulian ryanjulian Oct 29, 2018

Choose a reason for hiding this comment

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

is this still needed?

Copy link
Member Author

@CatherineSue CatherineSue Oct 30, 2018

Choose a reason for hiding this comment

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

Yes

@coveralls
Copy link

coveralls commented Oct 30, 2018 •
edited
Loading

Coverage Status

Coverage increased (+0.09%) to 65.171% when pulling 3589a48 on fix_pol_ent into e89ce7a on master.

@ryanjulian ryanjulian requested a review from zhanpenghe October 30, 2018 18:06
zhanpenghe
zhanpenghe reviewed Oct 30, 2018
View reviewed changes
zhanpenghe
zhanpenghe reviewed Oct 30, 2018
View reviewed changes
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.

Looks good to me.

zhanpenghe
zhanpenghe reviewed Oct 30, 2018
View reviewed changes
Copy link
Member

@zhanpenghe zhanpenghe left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Can you please check the example files or benchmarking comments? I believe there should be some changes with hyper parameters.

For not using negative log likelihood as entropy term, since you take out the mean now, the scale of the coefficient might be different too.

@ryanjulian
Copy link
Member

ryanjulian commented Oct 30, 2018

Do any of the examples even use entropy?

@zhanpenghe
Copy link
Member

zhanpenghe commented Oct 30, 2018 •
edited
Loading

policy_ent_coeff was by default set as 1e-2 before so any of the examples that does not explicit set it to 0 would use entropy.

@CatherineSue
Copy link
Member Author

CatherineSue commented Oct 30, 2018 •
edited
Loading

The default value of use_logli_entropy is True, so every related example and benchmark will be using negative log likelihood as entropy term if they don't set policy_ent_coeff to 0.

@CatherineSue CatherineSue force-pushed the fix_pol_ent branch 3 times, most recently from 3d832b6 to c8c02e8 Compare October 30, 2018 21:55
@CatherineSue
Copy link
Member Author

CatherineSue commented Oct 30, 2018

I changed the default policy_ent_coeff to 0.0.

zhanpenghe
zhanpenghe approved these changes Oct 30, 2018
View reviewed changes
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.

Ok. Please update the comments about benchmarking in the examples

Fix policy entropy
3589a48 
This commit fixes policy entropy. Currently it uses the following three
options:
• maximum entropy (augmented reward)
• with a logli estimator (-logli(action|policy)
• with the gradient turned off (tf.stop_gradient(entropy))
@CatherineSue CatherineSue merged commit 26c14a3 into master Oct 30, 2018
@zhanpenghe zhanpenghe deleted the fix_pol_ent branch October 31, 2018 17:58
@ryanjulian ryanjulian mentioned this pull request Nov 2, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ryanjulian ryanjulian ryanjulian approved these changes

@zhanpenghe zhanpenghe zhanpenghe approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@CatherineSue @coveralls @ryanjulian @zhanpenghe