-
Notifications
You must be signed in to change notification settings - Fork 419
FlowDistribution #88
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
base: master
Are you sure you want to change the base?
FlowDistribution #88
Conversation
# Conflicts: # tests/model/test_stochastic.py
Currently, I have only modified the Additionally, discussion on wether you guys like the |
Cool, I will start looking at this once I finished the matrix normal one. |
Back from rushing an icml paper. Sorry for being late but I'm reading the code now. |
Sure no worries, hope ICML went well. |
with zs.BayesianNet() as variational: | ||
lz_x = tf.layers.dense(tf.to_float(x), 500, activation=tf.nn.relu) | ||
lz_x = tf.layers.dense(lz_x, 500, activation=tf.nn.relu) | ||
z_mean = tf.layers.dense(lz_x, z_dim) | ||
z_logstd = tf.layers.dense(lz_x, z_dim) | ||
z = zs.Normal('z', z_mean, logstd=z_logstd, group_ndims=1, | ||
n_samples=n_particles) | ||
z = zs.NormalFlow('z', forward, mean=z_mean, logstd=z_logstd, group_ndims=1, |
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 feel NormalFlow
is a bit too restricted. And it also makes the internal Normal node invisible. I'd prefer sth. like
z_pre = zs.Normal('z_pre', ...)
z = zs.Flow('z', flow=zs.planar_normalizing_flow, n_iters=n_flows)
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.
However, this would require some extra internal machinery or require that the input to the flow is an instance of StochastcTensor
in order to still be able to use its sampling and log-probability methods
See :class:`~zhusuan.distributions.base.Distribution` for details. | ||
|
||
:param name: A string. The name of the `StochasticTensor`. Must be unique | ||
in the `BayesianNet` context. |
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 signature is incorrect
:param base: An instance of `Distribution` parametrizing the base distribution. | ||
:param forward: A forward function which describes how we transform the samples | ||
from the base distribution. The signature of the function should be: | ||
transformed, log_det = forward(base_samples) |
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.
We should restrict how the forward function works. Does it apply transformation on each value (of value_shape
) individually, or does it apply transformation on the group of values (of shape batch_shape[-group_ndims:] + value_shape
)? There needs some careful design.
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.
So, I think that the function signature should be on shape batch_shape[-group_ndims:] + value_shape
since essentially the groups provide the correct shape for i.i.d. variable. I have not met in practice diagonal flows and it would seem that if you use that you can change the group_ndims
return self.base.value_shape() | ||
|
||
def _get_value_shape(self): | ||
return self.base.get_value_shape() |
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.
As stated above, the new value shape is not always the same as the original one, e.g., in the VAE case.
return self.base.get_batch_shape() | ||
|
||
def _sample(self, n_samples): | ||
return self.sample_and_log_prob(n_samples)[0] |
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.
This seems to not follow our discussion here? Also the StochasticTensor
part.
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 I've decided to leave the _sample
be implemented as I circumvented the issue with tensor using the local_log_prob
property. If you still want I can make it not implemented, but I'm not sure it is needed.
tensor = self.tensor | ||
if not hasattr(self, '_local_log_prob'): | ||
self._local_log_prob = self.log_prob(tensor) | ||
return self._local_log_prob |
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.
Nice job. Meanwhile can you remove the log_prob(given)
function in StochasticTensor
? We should make StochasticTensor
more static and leave these functions to StochasticTensor.distribution.log_prob(given)
'planar_normalizing_flow', | ||
] | ||
|
||
|
||
def repeated_flow(base_flow_function, samples, n_iters, scope=None, |
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.
Nice refactor. Thanks for the improvements.
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.
After we sort out correctly all of the flow distribution details, I was intending to add at least a couple of more of the flows in the literature as well.
Following the conversation in #70