-
Notifications
You must be signed in to change notification settings - Fork 759
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
Hamiltonian Monte Carlo with Dual Averaging #728
base: master
Are you sure you want to change the base?
Conversation
@dustinvtran all checks have passed, could this PR be merged ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My sincere apologies for the delay. I finally had some time to re-read the NUTS paper and go through your implementation. Great job! (especially on the dynamic leapfrog implementation)
I only have minor suggestions below.
edward/inferences/hmcda.py
Outdated
step_size = self.find_good_eps() | ||
sess = get_session() | ||
init_op = tf.global_variables_initializer() | ||
sess.run(init_op) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The initialize op shouldn't be needed inside inference.initialize()
. It's called within the run()
method or alternatively must be called manually after you call inference.initialize()
on the algorithm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without this, I have: Attempting to use uninitialized value Variable
.
I can't making it work without :/
edward/inferences/hmcda.py
Outdated
The updates assume each Empirical random variable is directly | ||
parameterized by ``tf.Variable``s. | ||
""" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove emptyline
edward/inferences/hmcda.py
Outdated
"""Simulate Hamiltonian dynamics using a numerical integrator. | ||
Correct for the integrator's discretization error using an | ||
acceptance ratio. The initial value of espilon is heuristically chosen | ||
with Algorithm 4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
epsilon
, Algorithm 4.
edward/inferences/hmcda.py
Outdated
""" | ||
self.scope_iter = 0 # a convenient counter for log joint calculations | ||
|
||
# Find intial epsilon |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
initial
edward/inferences/hmcda.py
Outdated
Parameters | ||
---------- | ||
n_adapt : float | ||
Number of samples with adaption for epsilon |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adaptation
edward/inferences/hmcda.py
Outdated
# Accept or reject sample. | ||
u = Uniform().sample() | ||
alpha = tf.minimum(1.0, tf.exp(ratio)) | ||
accept = u < alpha |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comparing with tf.log(u) < ratio
should be more numerically stable than checking on the PDF scale.
edward/inferences/hmcda.py
Outdated
assign_ops.append(self.n_accept.assign_add(tf.where(accept, 1, 0))) | ||
return tf.group(*assign_ops) | ||
|
||
def do_not_adapt_step_size(self, alpha): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for methods built for internal implementation, prepend the name with _
.
edward/inferences/hmcda.py
Outdated
def do_not_adapt_step_size(self, alpha): | ||
# Do not adapt step size but assign last running averaged epsilon to epsilon | ||
assign_ops = [] | ||
assign_ops.append(self.H_B.assign_add(0.0).op) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason you add 0 to these variables? What happens if we don't use assign ops for them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tf.cond
arguments; true_fn
and false_fn
must have the same type of outputs, which is a list of ops in our case.
We could also do assign_ops.append(tf.assign(self.H_B, self.H_B).op)
.
I would be happy to receive any better idea.
Thanks for your feedbacks @dustinvtran ! I've fixed all your suggestions but the initialization issue. I dot know how to fix that. |
edward/inferences/hmcda.py
Outdated
Latent variable keys to samples. | ||
""" | ||
self.scope_iter += 1 | ||
scope = 'inference_' + str(id(self)) + '/' + str(self.scope_iter) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note we no longer use scope_iter
and str(id(self))
implementation for scopes. See the latest versions of hmc.py
and sghmc.py
where we use scope = tf.get_default_graph().unique_name("inference")
.
edward/inferences/hmcda.py
Outdated
assign_ops.append(self.n_accept.assign_add(tf.where(accept, 1, 0))) | ||
return tf.group(*assign_ops) | ||
|
||
def _do_not__adapt_step_size(self, alpha): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_do_not_adapt_step_size
Do you know which |
I have updated to Could it come from the empirical variables |
Upon further investigation, the issue is data-dependent initialization. The Not sure how to fix this just yet. |
tests/test-inferences/test_hmcda.py
Outdated
# analytic solution: N(loc=0.0, scale=\sqrt{1/51}=0.140) | ||
inference = ed.HMCDA({mu: qmu}, data={x: x_data}) | ||
inference.run(n_adapt=1000) | ||
print(qmu.mean().eval()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove print statements in test
edward/inferences/hmcda.py
Outdated
k = tf.constant(0) | ||
|
||
def while_condition(k, v_z_new, v_r_new, grad_log_joint): | ||
# Stop when k < n_steps |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
always use two space indent, including inside internal functions
Prints and indents fixed. Nice spotted for the initialization issue ! What about the workaround proposed by yaroslavvb (2sd comment) ? |
I hesitate because it (1) requires users to use a custom initialization scheme; and (2) depends on a When replacing the init op inside if variables is None:
# Force variables to be initialized after any variables they depend on.
from tensorflow.contrib import graph_editor as ge
def make_safe_initializer(var):
"""Returns initializer op that only runs for uninitialized ops."""
return tf.cond(tf.is_variable_initialized(var),
tf.no_op,
lambda: tf.assign(var, var.initial_value).op,
name="safe_init_" + var.op.name).op
safe_initializers = {}
for v in tf.global_variables():
safe_initializers[v.op.name] = make_safe_initializer(v)
g = tf.get_default_graph()
for v in tf.global_variables():
var_name = v.op.name
var_cache = g.get_operation_by_name(var_name + "/read")
ge.reroute.add_control_inputs(var_cache, [safe_initializers[var_name]])
init = tf.group(*safe_initializers.values())
else:
init = tf.variables_initializer(variables) |
Hello to all !
This PR partially solve this issue #541 which is about implementing NUTS. As proposed in this topic, starting with the Dual Averaging is a good start.
The implemented inference method is in
edward/inferences/hmcda.py
and the corresponding test intests/test-inferences/test_hmcda.py
.I hope you'll appreciate the PR ! :)