-
Notifications
You must be signed in to change notification settings - Fork 13
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
Add neural score estimation (NSE) #7
Conversation
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.
Hi! I have yet to run and play with the code, but here are some comments from just reading the code.
phi=(x, *self.parameters()), | ||
), | ||
base=DiagNormal(self.zeros, self.ones).expand(batch_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.
I am not sure if there is any definite API yet but I find a bit inconsistent that NPE
defines a sample
method based on its own self.flow
and here there is no sample
method implemented.
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.
Yes, I dislike it as well, but I am not sure of the best way to sample from the trained estimator yet. From my toy experiments sampling from the probability flow ODE gives good samples and is pretty fast, but Song et al. (2021) put forward some evidence that ancestral and reverse SDE sampling yields more qualitative samples.
So maybe the sample
method could be another sampling method than just return self.flow(x).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.
Yeah, I was more worried about the lack of consistency between the interfaces of the classes than about the particular implementation of the sample
interface.
Most comments have been addressed in the last commits. The description of NSE was mostly re-written. |
phi=(x, *self.parameters()), | ||
), | ||
base=DiagNormal(self.zeros, self.ones).expand(batch_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.
After experimenting with this part of the code, it would be nice to have a unified API such that nse.flow
can be passed directly to the coverage utils. This is currently not convenient because nse.flow.log_prob
is a noisy estimate of the log-density.
Thanks for the changes! I have experimented with the codebase and confirm that it works fine -- it produces good results on SLCP, and the coverage is reasonable. +1 for merging in its current state |
Targets #6