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

Should we switch to pyro flows? #336

Closed
michaeldeistler opened this issue Sep 21, 2020 · 8 comments
Closed

Should we switch to pyro flows? #336

michaeldeistler opened this issue Sep 21, 2020 · 8 comments
Labels
architecture Internal changes without API consequences enhancement New feature or request question Further information is requested

Comments

@michaeldeistler
Copy link
Contributor

Currently, we are relying on nflows for our flows. I think that pyro's flows look pretty great, see e.g. here for NSF.

They very nicely separate the transformer from the conditioner, see e.g. here for autoregressive splines.

Any reasons to stick to nflows?

@michaeldeistler michaeldeistler added enhancement New feature or request question Further information is requested architecture Internal changes without API consequences labels Sep 21, 2020
@jan-matthis
Copy link
Contributor

I agree but we'd have to make sure that performance and speed are matched. One step in that direction could be to check performance and speed of Pyro-based MAFs and NSFs on experiments/uci.py of https://github.com/bayesiains/nsf

@michaeldeistler
Copy link
Contributor Author

We will move to flowtorch after the hackathlon

@francois-rozet
Copy link

Hello 👋

I noticed you wanted to switch from nflows to flowtorch for your normalizing flows. I must warn you that flowtorch is still missing important features. Notably, there is currently no way to build conditional normalizing flows, as required for NPE.

Initially, our package lampe was relying on nflows for its normalizing flows, but we ran into some issues that we couldn't fix. We wanted to contribute to nflows but the project seemed abandoned. As alternatives, we considered pyro and flowtorch but neither of them were actually usable. Eventually, we chose to implement normalizing flows from scratch within lampe, with an approach similar to that of pyro but relying more on distributions and transformations provided by PyTorch.

Since then, the flow implementations have improved and new ones were added such as NSF, NAF and UMNN (both coupling and fully autoregressive). Recently, we decided to export our normalizing flows to a standalone package named Zuko. If you still want to switch from nflows, you might consider zuko 🔥

@michaeldeistler
Copy link
Contributor Author

Hi Francois,

thanks for the pointer, and we are indeed on the lookout for a good repo for flows. We'll have a look!

Michael
PS: amazing package name and logo

@plcrodrigues
Copy link
Contributor

Hi, I was wondering whether this issue could be considered as relevant to the SBI Hackathon 2024.

I think it would be great to have a flows backend base on @francois-rozet's awesome work on zuko. The package is actively maintained by François and would benefit from having more contributors.

Also, this migration could eventually lead to people switching between sbi and lampe more easily. In the long term, maybe the former could be seen as a package for end-users in applications while the latter as a package for prototyping new algorithms in the sbi literature ?

I don't know, maybe this sounds infeasible and not even pertinent to the current discussion, but I feel that relying on nflows (last update in 2020) or flowtorch (last update in 2022) would mean relying on code that is no longer actively maintained and putting the sbi package in a precarious situation.

@janfb
Copy link
Contributor

janfb commented Feb 28, 2024

Hi Pedro, thanks for your input!

Yes, it's relevant for the hackathon indeed! we are currently preparing to abstract away the dependency on only nflows (keep them, but allow for other density estimator packages, e.g., zuko). See #952 and #957

Our plan is to get this done before the hackathon to be able to integrate new density estimators during the hackathon more easily.

@janfb
Copy link
Contributor

janfb commented Mar 15, 2024

Is addressed in #957

@janfb janfb closed this as completed Apr 3, 2024
@janfb
Copy link
Contributor

janfb commented Apr 3, 2024

interface zuko is done. See also #1116

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
architecture Internal changes without API consequences enhancement New feature or request question Further information is requested
Projects
None yet
Development

No branches or pull requests

5 participants