-
Notifications
You must be signed in to change notification settings - Fork 761
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
Implementation of replica exchange MCMC #865
Conversation
I apologize. I'll fix it. |
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.
Looks great! Only some minor comments for now.
edward/inferences/__init__.py
Outdated
@@ -22,6 +22,7 @@ | |||
from edward.inferences.variational_inference import * | |||
from edward.inferences.wake_sleep import * | |||
from edward.inferences.wgan_inference import * | |||
from edward.inferences.replica_exchange_mc import * |
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.
can you add where the imports are in alphabetical order?
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.
OK. I'll fix.
from edward.util import check_latent_vars, copy | ||
|
||
|
||
class memory_lambda: |
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 implementation names, we recommend privates: i.e., _memory_lambda
(or _MemoryLambda
). Another possible name is _stateful_lambda
.
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.
OK. I'll fix.
|
||
class ReplicaExchangeMC(MonteCarlo): | ||
""" | ||
Replica Exchange MCMC(https://en.wikipedia.org/wiki/Parallel_tempering) |
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.
Cool! Is there a canonical citation we can use instead of the Wiki article?
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.
I think we should cite Swendsen and Wang(1986), Hukushima and Nemoto(1996).
I'll add the citations to bib.bib and this code.
#### Examples | ||
```python | ||
cat = Categorical(probs=[0.5,0.5]) | ||
x = Mixture(cat=cat, components= |
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 PEP8, need
x = Mixture(cat=cat, components=[
MultivariateNormal...,
....])
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.
OK. I'll fix.
def __init__(self, latent_vars, proposal_vars, data=None, | ||
betas=np.logspace(0, -2, 5), exchange_freq=0.1): | ||
"""Create an inference algorithm. | ||
Args: |
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.
add blankline inbetween args and top-level docstring
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.
OK. I'll fix.
# Exchange adjacent replicas at frequency of exchange_freq | ||
i = tf.random_uniform((), maxval=2, dtype=tf.int32) | ||
|
||
def c(i, new_replica_idx): |
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.
rename to cond
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.
OK. I'll fix.
def c(i, new_replica_idx): | ||
return tf.less(i, self.n_replica - 1) | ||
|
||
def b(i, new_replica_idx): |
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.
rename to body
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.
OK. I'll fix.
return tf.less(i, self.n_replica - 1) | ||
|
||
def b(i, new_replica_idx): | ||
return [tf.add(i, 2), self.replica_exchange(i, i + 1, replica_sample, |
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.
s/tf.add(i, 2)
/i + 2
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.
OK. I'll fix.
zip(six.iterkeys(new_sample), sample_values)} | ||
return sample, accept | ||
|
||
def replica_exchange(self, candi, candj, replica_sample, new_replica_idx): |
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.
rename to private method, _replica_exchange
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.
OK. I'll fix.
|
||
return tf.group(*assign_ops) | ||
|
||
def mh_sample(self, latent_vars, beta): |
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.
rename to private method _mh_sample
Also, since this is a direct copy of MH algorithm's build_update
, does it make sense to just build this class by inheriting from MetropolisHastings
?
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.
Thanks!
As you say, except for using inverse temperature beta
, this code is a copy of a part of MH algorithm's build_update
. However in MH algorithm's build_update
, sampling and updating are perform in the same function. In replica exchange method, exchanging must be perform between sampling and updating. Therefore, I think it is difficult to make ReplicaExchangeMC
by inheriting from current MetropolisHastings
.
To make ReplicaExchangeMC
by inheriting from current MetropolisHastings
,
It may be necessary to modify MetropolisHastings
. (ex. creating _mh_sample
)
And, replica exchange monte calro method doesn't necessarily have to use MH algorithm. Instead of MH algorithm, we can use Hamiltonian Monte Carlo and so on. So if possible, I'd like to make monte calro algorithms to use in ReplicaExchangeMC
selectable.
Thank you for reading.
It would be extremely helpful if I could receive your advice.
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.
I see. Your choice is great—let's leave as is for now. We can always do refactoring in a future PR to avoid that code duplication.
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.
Thanks! I understand.
Thank you for your quick and polite comments! Based on your comment, I corrected everything I can fix. But, the Travis CI check does not pass. |
Sure, restarting job. |
Thank you very much! And I'm happy that all checks passed. |
All looks great. Thanks again! |
#861
Happy to open this pull request. I implemented replica exchange MCMC. I would appreciate it if you would review and comment.