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

Better default arguments for MCMC, SNLE, SNPE. #910

Closed
michaeldeistler opened this issue Jan 16, 2024 · 10 comments · Fixed by #1221
Closed

Better default arguments for MCMC, SNLE, SNPE. #910

michaeldeistler opened this issue Jan 16, 2024 · 10 comments · Fixed by #1221
Assignees
Labels
documentation Improvements or additions to documentation hackathon

Comments

@michaeldeistler
Copy link
Contributor

michaeldeistler commented Jan 16, 2024

  • especially num_chains
  • vectorized sampler is not default
  • Add a tutorial
@michaeldeistler michaeldeistler changed the title High num_chains for MCMC as default? Better default arguments for MCMC? Jan 16, 2024
@janfb
Copy link
Contributor

janfb commented Jan 24, 2024

Note: slice_np_vectorized with a small thin, e.g., <5, results in 0 and inf samples ❗ even for Gaussian examples and only for num_chains>1.

Update: thinning does not seem to be the problem. if thinning is small it just increases the likelihood of hitting an inf or NaN value.
The main reason seems to be the number of warmup steps. If it is too small, chains have not converged and samples diverge (why?)

@janfb janfb added documentation Improvements or additions to documentation hackathon labels Jan 24, 2024
@janfb janfb mentioned this issue Jan 30, 2024
3 tasks
@janfb janfb added this to the Pre Hackathon 2024 milestone Feb 6, 2024
@gmoss13
Copy link
Contributor

gmoss13 commented Feb 29, 2024

Additional context: The default parameters are defined here. Following discussion in #924 - a good suggestion for the default parameters for MCMCPosterior might be:
method="slice_np_vectorized", num_chains=20, warmup_steps=50, thin=5
It could be useful to add a warning that sampling could be slow if the user sets the method to be non-vectorised, and num_workers is small.

@gmoss13
Copy link
Contributor

gmoss13 commented Feb 29, 2024

Once #908 is addressed there might be even better default args

@janfb
Copy link
Contributor

janfb commented Mar 22, 2024

@famura can you please add the new info about best practice regarding thinning that you mentioned earlier today?
That'd be great! 🙏

@famura
Copy link
Contributor

famura commented Mar 22, 2024

@famura can you please add the new info about best practice regarding thinning that you mentioned earlier today?
That'd be great! 🙏

Sure, where do you want me to add it? in the top level class?

@janfb
Copy link
Contributor

janfb commented Mar 22, 2024

@famura can you please add the new info about best practice regarding thinning that you mentioned earlier today?
That'd be great! 🙏

Sure, where do you want me to add it? in the top level class?

No, just post a link to the resource giving more details. Then we can use it when we decide for better default args later

@famura
Copy link
Contributor

famura commented Mar 22, 2024

So, @felixp8 and I came across this post. Moreover, @michaeldeistler and @sethaxen discussed with the somewhat confident result that we can set thinning to 1 by default. However, since this will most likely lead to results that differ from the benchmark, we came up with the solution that we set thin to -1 by default, catch that and set it then to 1 and raise a warning that the default has changed.

Is that the kind of information you were looking for?

@famura
Copy link
Contributor

famura commented Mar 22, 2024

On a different note @sethaxen said that num_chains=4 would be a good default

@famura
Copy link
Contributor

famura commented Mar 22, 2024

Linked to PR #987 and PR #1053 which has the changes

@janfb
Copy link
Contributor

janfb commented Jul 22, 2024

TODO:

  • switch to slice_np_vectorized as default arg (in posterior classes and build_posterior methods):
  • switch to num_chains=10 as default
  • set MAF for SNLE and NSF for SNPE.

A bit unrelated but I also suggest switching training defaults to

  • training_batch_size=100 or even 200

@janfb janfb changed the title Better default arguments for MCMC? Better default arguments for MCMC, SNLE, SNPE. Aug 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation hackathon
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants