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

Linear Regression Example #10

Merged
merged 2 commits into from Aug 2, 2017

Conversation

Projects
None yet
3 participants
@LeahPrice
Collaborator

LeahPrice commented Aug 1, 2017

This simple example shows how to use the package to sample from the posterior distribution of a static Bayesian model and to estimate the model evidence. The example was originally presented in the literature as a model choice problem which is simple enough for the true log evidence estimates (and Bayes factor) to be obtainable via numerical integration. Users can select which model they wish to perform inference on (model 1 or 2).

Linear Regression Example
This simple example shows how to use the package to sample from the posterior distribution of a static Bayesian model and to estimate the model evidence. The example was originally presented in the literature as a model choice problem which is simple enough for the true log evidence estimates (and Bayes factor) to be obtainable via numerical integration. Users can select which model they wish to perform inference on (model 1 or 2).
@adamjohansen

This looks good to me. There are a few specific comments, but nothing that needs any changes to code other than perhaps one or two of the comments at this stage.

Show outdated Hide outdated R/LinReg.R
Show outdated Hide outdated R/LinRegLA.R
namespace LinReg {
class rad_state

This comment has been minimized.

@adamjohansen

adamjohansen Aug 1, 2017

Collaborator

Just a remark and I wouldn't necessarily change anything here, but in general I don't know if there's a need to define a class which just contains another type. If it makes the code more readable then fair enough, but an arma::vec or std::pair of arma::vec would be perfectly reasonable classes to use.

@adamjohansen

adamjohansen Aug 1, 2017

Collaborator

Just a remark and I wouldn't necessarily change anything here, but in general I don't know if there's a need to define a class which just contains another type. If it makes the code more readable then fair enough, but an arma::vec or std::pair of arma::vec would be perfectly reasonable classes to use.

@@ -0,0 +1,56 @@
// -*- mode: C++; c-indent-level: 4; c-basic-offset: 4; indent-tabs-mode: nil; -*-
//
// LinReg_LA.h: Rcpp wrapper for SMC library -- A simple example for estimating

This comment has been minimized.

@adamjohansen

adamjohansen Aug 1, 2017

Collaborator

It's quite possible that the answer is "yes", and it might be cleaner this way, but is there a good reason for having completely separate implementations of the two approaches to linear regression?

There seems to be an amount of code overlap and my first instinct would have been to share common code between the two (defining two classes with the same name and content in different namespaces just feels like harder work than necessary, but it at least gives a good degree of modularity).

@adamjohansen

adamjohansen Aug 1, 2017

Collaborator

It's quite possible that the answer is "yes", and it might be cleaner this way, but is there a good reason for having completely separate implementations of the two approaches to linear regression?

There seems to be an amount of code overlap and my first instinct would have been to share common code between the two (defining two classes with the same name and content in different namespaces just feels like harder work than necessary, but it at least gives a good degree of modularity).

This comment has been minimized.

@LeahPrice

LeahPrice Aug 2, 2017

Collaborator

I mostly just did that to keep things clean and easy to follow for now and for later when we add in the adaptation. I could combine them and have helper functions to get the prior, f(y_t|theta) and f(y_{1_t}|theta) but it might make the code a little harder to read. I'm also keeping track of the log likelihood and log prior in the likelihood annealing example but not in data annealing because that wouldn't really make sense.

I don't have a particularly strong preference either way -- combining three sets of files (data annealing, likelihood annealing and likelihood annealing with adaptation) might make it harder to read, but at the same time having three sets of files for one example seems over the top.

@LeahPrice

LeahPrice Aug 2, 2017

Collaborator

I mostly just did that to keep things clean and easy to follow for now and for later when we add in the adaptation. I could combine them and have helper functions to get the prior, f(y_t|theta) and f(y_{1_t}|theta) but it might make the code a little harder to read. I'm also keeping track of the log likelihood and log prior in the likelihood annealing example but not in data annealing because that wouldn't really make sense.

I don't have a particularly strong preference either way -- combining three sets of files (data annealing, likelihood annealing and likelihood annealing with adaptation) might make it harder to read, but at the same time having three sets of files for one example seems over the top.

This comment has been minimized.

@adamjohansen

adamjohansen Aug 2, 2017

Collaborator

Fair enough, I can see an argument for keeping the two implementations quite separate to make it very easy for someone looking at the code to see exactly what it does. I really just wanted to check that the existing code hadn't given the impression that there needed to be completely separate source files for each principal function or anything like that.

If I was doing this in practice I'd have written one function which evaluated the likelihood over a specified subset of the data and then just called that appropriately for different implementations, but that's not the only sensible way of doing things and might not be the clearest for someone new to the code.

@adamjohansen

adamjohansen Aug 2, 2017

Collaborator

Fair enough, I can see an argument for keeping the two implementations quite separate to make it very easy for someone looking at the code to see exactly what it does. I really just wanted to check that the existing code hadn't given the impression that there needed to be completely separate source files for each principal function or anything like that.

If I was doing this in practice I'd have written one function which evaluated the likelihood over a specified subset of the data and then just called that appropriately for different implementations, but that's not the only sensible way of doing things and might not be the clearest for someone new to the code.

Show outdated Hide outdated man/LinReg.Rd
@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Aug 1, 2017

Collaborator

I look forward to seeing this example once finalized. That is really nice to have too.

Collaborator

eddelbuettel commented Aug 1, 2017

I look forward to seeing this example once finalized. That is really nice to have too.

Minor documentation changes
Giving more detail about the two competing models for the radiata pine regression example and removing obsolete comments about data handling.
@adamjohansen

Thanks; that should be enough for someone new to this to understand (qualitatively) what's going on I think.

This all looks good to me now.

@eddelbuettel eddelbuettel merged commit cfa1ffb into rcppsmc:master Aug 2, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Aug 2, 2017

Collaborator

@LeahPrice I just added to commits for minor polish. R CMD check was pointing out missing globals, missing imports and the ChangeLog was not quite in the right format (Emacs knows best).

Thanks for keeping the ChangeLog up to date,

Collaborator

eddelbuettel commented Aug 2, 2017

@LeahPrice I just added to commits for minor polish. R CMD check was pointing out missing globals, missing imports and the ChangeLog was not quite in the right format (Emacs knows best).

Thanks for keeping the ChangeLog up to date,

@LeahPrice LeahPrice deleted the LeahPrice:LinReg branch Aug 3, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment