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

DQN example huber loss and typo, issue #82 #83

Merged
merged 3 commits into from Jun 9, 2020

Conversation

dcharrezt
Copy link
Contributor

@dcharrezt dcharrezt commented Jun 9, 2020

Issue #82
Adding baselines package need to run the notebook,
Correcting small typo
Changing huber loss function for tf2

@dcharrezt dcharrezt changed the title DQN example huber loss and typo, issue #84 DQN example huber loss and typo, issue #82 Jun 9, 2020
Copy link
Member

@fchollet fchollet left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

@@ -209,7 +209,7 @@
q_action = tf.reduce_sum(tf.multiply(q_values, masks), axis=1)
# Calculate loss between new Q-value and old Q-value
# Clip the deltas using huber loss for stability
loss = keras.losses.huber(updated_q_values, q_action)
loss = tf.compat.v1.losses.huber_loss(updated_q_values, q_action)
Copy link
Member

@fchollet fchollet Jun 9, 2020

Choose a reason for hiding this comment

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

This should be tf.reduce_mean(keras.losses.huber(...)), see https://keras.io/api/losses/regression_losses/#huber-function

Or alternatively

loss_fn = Huber()  # Outside the loop
loss = loss_fn(...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the correction, since keras.losses.huber was added in tf-nightly, I am adding the command to install it in the colab.

Copy link
Member

Choose a reason for hiding this comment

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

Please remove the commands, the file already explicitly mentions that you need to install tf-nightly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those commands were removed! thanks for pointing that out.

Copy link
Member

@fchollet fchollet left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@fchollet fchollet merged commit 442afd5 into keras-team:master Jun 9, 2020
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

2 participants