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

Add tutorials using normalizing flows #3302

Merged
merged 15 commits into from
Jan 14, 2024
Merged

Add tutorials using normalizing flows #3302

merged 15 commits into from
Jan 14, 2024

Conversation

francois-rozet
Copy link
Contributor

@francois-rozet francois-rozet commented Dec 5, 2023

This PR adds tutorials (SVI and VAE) using normalizing flows. The normalizing flow implementations are loaded from the Zuko library. By the way, I think it could be interesting to add a "compatibility with external tools" section to the examples.

In addition (as discussed in #3297), this PR also updates the normalizing flow tutorial to mention that Pyro's built-in flow API is not developed anymore. I also fixed a mistake ($\det | \frac{\partial f}{\partial x} |$ should be $| \det \frac{\partial f}{\partial x} |$) in the math and cleaned up a bit.

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

Copy link
Collaborator

@martinjankowiak martinjankowiak left a comment

Choose a reason for hiding this comment

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

the two usage tutorials look great but i think it'd be much better if you first made a separate PR for Zuko2Pyro which could sit in distributions or in contrib.zuko. this way this class can be tested and imported into your example notebooks

@francois-rozet
Copy link
Contributor Author

the two usage tutorials look great but i think it'd be much better if you first made a separate PR for Zuko2Pyro which could sit in distributions or in contrib.zuko. this way this class can be tested and imported into your example notebooks

I didn't know I could do that! That would be much better. In fact, this is actually a torch-to-Pyro class (Zuko uses plain PyTorch distributions), so it could be interesting as part of pyro.distributions.

@martinjankowiak
Copy link
Collaborator

@francois-rozet isn't rsample_and_log_prob zuko-specific?

@francois-rozet
Copy link
Contributor Author

francois-rozet commented Dec 7, 2023

Indeed, but the class still works if dist does not implement rsample_and_log_prob. So it is not actually necessary to import zuko. Do you have a preference for pyro.distributions or contrib.zuko ?

BTW, Transform.call_and_ladj (also Zuko-specific) and Distribution.rsample_and_log_prob could be worth moving upstream (to torch itself), as they allow sampling and scoring without cache (and barely increase API complexity).

@martinjankowiak
Copy link
Collaborator

@fritzo do you have a preference between pyro.distributions and contrib.zuko? i do not. the former is probably more discoverable...

@martinjankowiak
Copy link
Collaborator

note you can put a test for your contrib code in tests/contrib

pyro/contrib/zuko.py Outdated Show resolved Hide resolved
pyro/contrib/zuko.py Show resolved Hide resolved
tests/contrib/test_zuko.py Outdated Show resolved Hide resolved
tutorial/source/normalizing_flows_intro.ipynb Outdated Show resolved Hide resolved
tutorial/source/normalizing_flows_intro.ipynb Show resolved Hide resolved
tutorial/source/svi_flow_guide.ipynb Show resolved Hide resolved
tutorial/source/svi_flow_guide.ipynb Outdated Show resolved Hide resolved
@martinjankowiak
Copy link
Collaborator

@fritzo do we need to add zuko to EXTRAS_REQUIRE?

@francois-rozet
Copy link
Contributor Author

@martinjankowiak I have addressed your comments (at least the ones I could). I don't think it is necessary to add zuko in EXTRAS_REQUIRE as even contrib/zuko.py does not rely on it. However, it seems like doctest tries to run the example I added in the docstring and fails because it does not import zuko. I can modify the example to be a code-block, which should not be executed.

martinjankowiak
martinjankowiak previously approved these changes Jan 4, 2024
Copy link
Collaborator

@martinjankowiak martinjankowiak left a comment

Choose a reason for hiding this comment

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

lgtm. @fritzo could you please take a final pass, especially w.r.t. caching logic?

Copy link
Member

@fritzo fritzo left a comment

Choose a reason for hiding this comment

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

@francois-rozet thank you for cleaning up our existing tutorials and for adding this new tutorial! I think these will help lots of users provide exposure for Zuko. And thanks for your patience in this very slow holiday review cycle. I think we can push a Pyro 1.9 release soon after this merges.

LGTM, subject to @martinjankowiak's review and a couple nits.

pyro/contrib/zuko.py Outdated Show resolved Hide resolved
pyro/contrib/zuko.py Show resolved Hide resolved
tests/contrib/test_zuko.py Outdated Show resolved Hide resolved
@francois-rozet
Copy link
Contributor Author

francois-rozet commented Jan 13, 2024

I addressed your comments, and I think the tests have passed (went to bed before the end).

Nope i fucked up my dummy test. Fixing that tomorrow.

@martinjankowiak
Copy link
Collaborator

lgtm apart from possibly adding a rsample_and_log_prob test

@francois-rozet
Copy link
Contributor Author

francois-rozet commented Jan 13, 2024

apart from possibly adding a rsample_and_log_prob test

I added a test already.

@martinjankowiak
Copy link
Collaborator

sorry i missed that thanks

@fritzo fritzo merged commit 4f17274 into pyro-ppl:dev Jan 14, 2024
9 checks passed
@francois-rozet
Copy link
Contributor Author

🎉

@martinjankowiak
Copy link
Collaborator

thanks @francois-rozet !!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants