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

initial gumbel distribution and bijection #36

Merged
merged 14 commits into from
Jan 27, 2022

Conversation

kashif
Copy link
Contributor

@kashif kashif commented Jul 27, 2021

for issue #21

Copy link
Collaborator

@franrruiz franrruiz left a comment

Choose a reason for hiding this comment

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

Very nice, thank you! I left some (mostly minor) comments. I would also suggest to add a gumbel_cdf_test.py file to test that the bijector behaves as intended.

distrax/_src/bijectors/gumbel.py Outdated Show resolved Hide resolved
distrax/_src/bijectors/gumbel.py Outdated Show resolved Hide resolved
distrax/_src/bijectors/gumbel.py Outdated Show resolved Hide resolved
distrax/_src/distributions/gumbel.py Outdated Show resolved Hide resolved
distrax/_src/distributions/gumbel.py Outdated Show resolved Hide resolved
distrax/_src/distributions/gumbel.py Show resolved Hide resolved
distrax/_src/distributions/gumbel_test.py Outdated Show resolved Hide resolved
distrax/_src/distributions/gumbel.py Outdated Show resolved Hide resolved
distrax/_src/distributions/gumbel.py Outdated Show resolved Hide resolved
distrax/_src/distributions/gumbel_test.py Outdated Show resolved Hide resolved
@kashif
Copy link
Contributor Author

kashif commented Aug 1, 2021

@franrruiz thanks for the review! I appreciate the time you spend on it and I'll get it fixed.

@hbq1 hbq1 added the cla: yes copybara label for automatic import label Sep 13, 2021
@franrruiz
Copy link
Collaborator

Hi Kashif - Just following up on the status of this CL. It's really nice and I'd like to have it imported to the library. Are there are any outstanding comments for which you'd like to get some input? I can see that there seem to be 2 tests that aren't passing which seem to refer to the number of arguments of the entropy method - that should be easily fixable but let me know if it's actually harder than it looks like.

@kashif
Copy link
Contributor Author

kashif commented Nov 8, 2021

Ok cool I’ll have a look and fix it up.

@mblondel mblondel mentioned this pull request Nov 12, 2021
@kashif
Copy link
Contributor Author

kashif commented Jan 25, 2022

@franrruiz I'm really stuck trying to figure out why the tests are failing... would you have a few mins to have a look and help me out? thanks!

@copybara-service copybara-service bot merged commit 5b90578 into google-deepmind:master Jan 27, 2022
@kashif kashif deleted the gumbel branch January 27, 2022 11:57
@kashif
Copy link
Contributor Author

kashif commented Jan 27, 2022

thanks! I appreciate it!

@franrruiz
Copy link
Collaborator

My pleasure! Just a quick update on why the tests were failing:

  1. Most tests failed because of TFP Gumbel. In particular, the tests use both Distrax and TFP distributions to check that they agree. But when creating the TFP distribution, the loc and scale parameters needed to be numpy arrays of type np.float32; otherwise an error was raised.

  2. Some other tests failed because the self._batch_size property of the Gumbel distribution was set to None when calling to the super.__init__ method. To fix this, I just moved the line where we set self._batch_size to appear after the call to the __init__ method of the superclass.

  3. As a minor point, there was a jnp.lgamma that should have been jax.lax.lgamma.

Besides this, we are planning to make a few more updates to the distribution and bijector before exposing it in the library. It'll be ready soon :)

@franrruiz
Copy link
Collaborator

Thank you @kashif for taking the initiative to write this addition to the library!

@kashif
Copy link
Contributor Author

kashif commented Jan 27, 2022

awesome! I'll take these lessons for the next distribution/transformation I implement! Very useful!

copybara-service bot pushed a commit that referenced this pull request Jan 28, 2022
copybara-service bot pushed a commit that referenced this pull request Jan 28, 2022
copybara-service bot pushed a commit that referenced this pull request Jan 28, 2022
copybara-service bot pushed a commit that referenced this pull request Jan 28, 2022
copybara-service bot pushed a commit that referenced this pull request Jan 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes copybara label for automatic import
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants