-
Notifications
You must be signed in to change notification settings - Fork 101
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
Bayesian interface -- timing model + white noise #1407
Conversation
…tation coming soon!)
I'll try to review this more closely, but with a quick look this looks really great & useful. My main comment would be to fix some of the formatting to make: Also, your example notebook looks really good, but I assume it won't be included in the built tutorials, right? Can you maybe then include some of the basic usage commands in the docstring since otherwise the notebook can be hard to find? Those don't get run, so there is no penalty for a complex calculation. Or maybe there's a better place to put that. |
param = getattr(model, par) | ||
param_min = float(param.value - 10 * param.uncertainty_value) | ||
param_span = float(20 * param.uncertainty_value) | ||
param.prior = Prior(uniform(param_min, param_span)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So you are setting an attribute that persist in the model object? Can you also include in the example how you set the priors with the separate dictionary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So you are setting an attribute that persist in the model object?
The model
attribute in BayesianTiming
is supposed to be a clone of the input model
. So line 39 should have been param = getattr(self.model, par)
. I have fixed this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have included the prior_info
usage in bayesian-example-NGC6440E.py
and a new test for this in test_bayesian.py
.
f"Unbounded uniform priors are not supported. (param : {param.name})" | ||
) | ||
|
||
def _decide_likelihood_method(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this work for both narrowband and Wideband data?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It works only works with narrow band now. I haven't tested with wideband data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, probably worth putting in the docs (& an issue?) and maybe checking when the toas are fed in?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
checking when the toas are fed in?
Yes... I think this is needed. I'll implement this. Maybe throw a NotImplementedError upon encountering wideband TOAs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implemented.
Also (very minor) please include a CHANGELOG entry. |
src/pint/bayesian.py
Outdated
3. Currently, only uniform and normal distributions are supported in prior_info. More | ||
general priors should be set directly in the TimingModel object before creating the | ||
BayesianTiming object. Here is an example prior_info object: | ||
>>> prior_info = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code block is still not formatting correctly for me at https://nanograv-pint--1407.org.readthedocs.build/en/1407/_autosummary/pint.bayesian.BayesianTiming.html. Maybe add in a blank line and indent further?
I think the python code blocks don't work (or at least I've had issues with those in the past, especially when also trying other languages). I would suggest just trying a blank line and extra indent. But we can also probably defer some of the final formatting until later. |
I have just made that example into a single line and pointed the user to the bayesian example script in the docstring. If this doesn't work either let us just remove that one line and proceed. |
BTW is there a way to build the readthedocs locally? |
Yes, look at https://nanograv-pint--1407.org.readthedocs.build/en/1407/contributing.html#write-documentation. |
A bayesian interface that allows the user to use PINT in combination with any sampler of their choice (both MCMC and nested samplers).
This PR implements the
BayesianTiming
class that provides the following methods:At present, this only works with white noise models and does not support correlated noise models.
See the attachment for a sample posterior plot obtained using this interface and the NGC6440E sample data.

I have checked that the result is consistent with the ENTERPRISE result.
Work in progress