Skip to content

Conversation

@MilesCranmer
Copy link
Contributor

@MilesCranmer MilesCranmer commented Jul 26, 2020

This is a work in progress PR.

I'm interested in hearing your comments. I performed some simple changes throughout the codebase to make it compatible for Python 3.6. The reason 3.6 would be of special interest is because the popular ML platform "Google Colaboratory" (used for freely demonstrating and sharing work requiring GPUs + a complex build environment) is exclusively on Python 3.6.

Changes include:

  • Removed all from __future__ import annotations statements.
  • Fixed all statements like import sbi.utils as utils to from sbi import utils as utils, since the first is incompatible with 3.6.
  • Also fixed this with nflows by changing nflows version from v0.11 to git commit bayesiains/nflows@84dc029 in setup.py (so it installs from git rather than from pypi). From a skim through the commit history, no major changes were added from v0.11 to this commit.
  • Imported common types for all files which use type-checking. Many files used types which were not directly imported, so I just added all the common types to all files.

@MilesCranmer
Copy link
Contributor Author

Example: you can go to the link:

https://colab.research.google.com/github/mackelab/sbi/blob/checkpointing/tutorial/00_getting_started.ipynb

and it will run the tutorial in-browser. Just run:

!pip install git+https://github.com/MilesCranmer/sbi

at the top.

@alvorithm
Copy link
Contributor

After a first quick look - thank you for the very granular commits and taking up the task of making sbi run with Python 3.6!

I am surprised that the type imports were absent - how has this passed our CI tests in the last days?

There's only one aspect of what I've seen that I would like to change: depending on a git version of nflows, but I guess @arturbekasov wouldn't mind to cut a 0.12 release to make nflows compatible with Python 3.6?

@arturbekasov
Copy link

I've cut a 0.12 release for nflows, with support for Python 3.6. I was going to wait until we fixed some other issues, but those are taking more time than anticipated, so might as well not block you guys here. Hope this helps.

@MilesCranmer
Copy link
Contributor Author

Awesome, thanks @arturbekasov! That's great. And thanks @Meteore for the quick review!

So it looks like the fact that these are passing might actually a long-running bug in Python: python/mypy#999 (essentially List/Set/Dict were mistakenly treated as aliases to list/set/dict for a while, which are auto-imported), which was fixed with this merge: python/mypy#2863. I'm not sure which Python versions the bug entered but maybe the CI is running a Python version that imports List/Dict/Set types by default, and the Google Colab Python does not.

@MilesCranmer
Copy link
Contributor Author

So everything is working in colab (https://colab.research.google.com/github/mackelab/sbi/blob/checkpointing/tutorial/00_getting_started.ipynb) with the pip version of nflows installed. pip gives a message about pyknos requiring nflows==0.11 still (I suppose its requirements will need to be updated too?), but it doesn't actually throw an error and the code all runs fine.

Thanks again,
Miles

@alvorithm
Copy link
Contributor

@MilesCranmer this is really excellent and we are very grateful. I hope you are set for tomorrow's tutorial and we will be making a new release ASAP with Py3.6 support (we'll take care of the pip req. when we integrate your PR). Thanks again and please tell attendees to give us feedback; we're excited to welcome more cosmologists and learn more about specific needs of the field!

@MilesCranmer
Copy link
Contributor Author

Everything is good to go, thanks so much @Meteore! I will also be sure to ask for feedback from attendees. I think LFI is picking up steam in Cosmology which is great, and this package looks like a perfect framework for the community.

Cheers,
Miles

@jan-matthis jan-matthis merged commit e963c29 into sbi-dev:main Jul 27, 2020
@jan-matthis
Copy link
Contributor

Thank you for the PR!

We've released the changes as v0.10.2:
https://github.com/mackelab/sbi/releases/tag/v0.10.2

@MilesCranmer
Copy link
Contributor Author

Awesome, thanks!

@MilesCranmer
Copy link
Contributor Author

MilesCranmer commented Jul 27, 2020

@Meteore following up with feedback from the tutorial. Btw, I made an extended tutorial based off of 00_getting_started.ipynb if you'd like me to put it somewhere. Walks through a bit of the ideas behind ABC + LFI + normalizing flows. Also does a quick PyTorch demo (I'm trying to introduce more astronomers to it as a general sci computing package).

They really liked the framework and how easy it was to use!

Some issues we encountered:

  • Seems like an error message is given since pyknos expects nflows to be 0.11. Exit code is still 0 though.
  • It seems .log_prob accepts a vector of samples happily for SNPE, but seems to fail silently for SNRE and SNLE and returns a constant value in the same shape as samples. From looking at the code, it seems like it is just built for a single point sample (and single observation value - though I already work with that by manually summing log-prob).
  • I see it noted elsewhere in the repo, but it would be great to have GPU support! Or more generally, a way to pass all kwargs through infer(...) to the specific model used.
  • Generally it would be nice if the samples and observations were assumed to be batched (or at least a flag "assume_batched" we can set), and the function automatically summed log_prob.
  • I think many would be interested in having a convolutional encoder available (or maybe a way to pass an arbitrary encoder?), as a lot of the data in astro is raw images & time series & density fields which we'd like to infer parameters from.

Cheers!
Miles

@michaeldeistler
Copy link
Contributor

Hi Miles,

thanks for all the feedback!

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.

5 participants