Skip to content
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

Detach sample and log_prob from neural net #17

Merged
merged 1 commit into from
Mar 30, 2021
Merged

Conversation

michaeldeistler
Copy link
Contributor

This PR provides methods that allow to sample() and log_prob() from a MoG when already knowing the precision and means. It's a simple refactor, but it will save us a lot of code repetition here.

Copy link
Contributor

@janfb janfb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks!
Can be merged once the two comments are addressed.

Return the log-probability of `inputs` under a MoG with specified parameters.

Unlike the `log_prob()` method, this method is fully detached from the neural
network and is a `staticmethod`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would remove the "and is a static method", this is clear from the code, no?
Instead I would add why this is useful, e.g., add sth like "can be used independent of the neural net in case the MoG parameters are already known"?

"""
Return samples of a MoG with specified parameters.

Unlike the `sample()` method, this method is fully detached from the neural
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same addition as for the log_prob_mog docstring.

@michaeldeistler
Copy link
Contributor Author

Totally agree with both comments, thanks!

@michaeldeistler michaeldeistler merged commit 613e6cc into main Mar 30, 2021
@michaeldeistler michaeldeistler deleted the refactor-mdn branch March 30, 2021 12:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants