-
Notifications
You must be signed in to change notification settings - Fork 378
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
Reporting bugs/errors in lecture3_flow_models_demos.ipynb
#7
Comments
I think I understand this part. Since |
Hi @vinsis - some thoughts on the bugs mentioned above.
I think the model is just PixelCNN + Mixture of Gaussian. It's not a "flow" as such. In the loss function, In general, I am curious if flows are ever used to sample from original distribution at all! They are good for inference, and plotting Pinging @wilson1yan @alexlioralexli for help. Thanks! |
For 4., it is sampling from the learned distribution of Side-note: It is a valid flow, and the terms do relate to the actual jacobian diagonal terms, as the log_derivative of the CDF is the log of the PDF. Note that if our target I'm not sure what you mean by |
The way log prob is calculated seems faulty to me @wilson1yan After the step But when calculating the log_prob,
which makes the shape of This makes I implemented this flow myself and am struggling to get decent results. So I won't be surprised if I missed something elementary. Edit: Just ran the notebook myself and |
First of all thank you for making all of the lectures and other content public. This is really helpful.
I took a look at the demo implementations for lecture 3 and found some bugs which I am reporting here:
1. In
Demo 3
, the.flow()
method inclass ConditionalMixtureCDFFlow(nn.Module):
has the following signature:However when
.flow()
is called by.invert()
method, the conditioncond
is not passed to.flow()
:2. In
Demo 4
the.forward()
method ofMaskConv2d
never usescond
orbatch_size
:So it has no effect when it is called by the
.forward()
method ofAutoregressiveFlowPixelCNN
like so:3. In
Demo 4
the.nll()
method of AutoregressiveFlowPixelCNN does not take exponential oflog_prob
and useweights
when calculatinglog_det_jacobian
:I think it should be something like:
I actually have lots of questions about why
.nll()
is implemented the way it is. Why the need to.unsqueeze(1).repeat(...)
rather than just multiplying it the standard way? Where is thebase_dist
that forces the output of the transformed variables to have a known distribution?Looking at the
.sample()
method it seems theweights
are used to selectmean
andvar
for a sample. But how are they learnt? Regardless of how it's being used, theweights
are not used in the.nll()
function and thus should not be calculated.Please let me know if I am missing something here.
The text was updated successfully, but these errors were encountered: