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

hardcoded uniform draw ranges #154

Open
paulthebaker opened this issue Nov 15, 2021 · 3 comments
Open

hardcoded uniform draw ranges #154

paulthebaker opened this issue Nov 15, 2021 · 3 comments
Labels
bug Something isn't working

Comments

@paulthebaker
Copy link
Member

the problem

In sampler.py the many prior draws correctly use enterprise.Parameter.sample() to draw from the prior. Other prior-like moves, draw_from_X_log_uniform_distribution, don't have that option and are currently hardcoded to a fixed range.

The many log-uniform amplitude draws were added because prior draws from a LinearExp prior are super inefficient. They are currently added to the mix whenever the named parameter is in the model. When the log-amplitude prior is Uniform the prior draw is log-uniform, so these moves are not really needed. Regular old prior draws are fine.

The log-uniform draws all have hard coded limits that match the default prior range. For example the standard CRN/GWB draw uses -18 to -14 (see sampler.py line 369). This agrees with the default amplitude prior for fixed gamma=13/3 runs, but not for varied gamma runs! The proposal returns logQxy = 0, assuming it covers the prior range, but it may not. If the prior range does not match the proposal range the proposal breaks detailed balance!

posible solution

The first mitigation step is to use regular prior draws for Uniform params. True prior draws will always pull from the correct range. We should only add these targeted proposals to upper limit runs that use LinearExp priors.

When setup_sampler is checking parameters and adding proposals to the cycle, it could check the actual prior ranges. Currently this is only accessible from the enterprise.Parameter.__repr__ string, which is a pain.

enterprise.Parameter could make the prior parameters accessible. This would mean the log-uniform draws (or any other) could directly use the actual prior range. This means we'd have to modify the enterprise.Uniform class factory (and hopefully Normal and others too).

@paulthebaker paulthebaker added the bug Something isn't working label Nov 15, 2021
@paulthebaker
Copy link
Member Author

see also #35...

@paulthebaker
Copy link
Member Author

paulthebaker commented Nov 15, 2021

We can probably eliminate most of the duplicate code in the draw_from_X_prior functions. In setup_sampler we can use the draw_from_par_prior function with different lists of parameters.

draw_from_par_log_uniform currently requires a dictionary of parameter names and ranges as input, it does not automatically figure out the prior range.

@paulthebaker
Copy link
Member Author

The prior range is accessible as a hidden property of of the enterprise.Parameter.prior function, because it is wrapped with enterprise.Function. So if param is a LinearExp(-18, -11) object, then

>>> param.prior._defaults
{'pmin':-18, 'pmin':-11}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

1 participant