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

Log Ratio of Normalizing Constants #7

Merged
merged 2 commits into from Jul 27, 2017
Merged

Conversation

@LeahPrice
Copy link
Collaborator

@LeahPrice LeahPrice commented Jul 27, 2017

Adding estimators of the log ratio of normalizing constant based on

  • the standard SMC estimator
  • path sampling estimator with 1st and 2nd order trapezoidal integration
Adding estimators of the log ratio of normalising constant based on
- the standard SMC estimator
- path sampling estimator with 1st and 2nd order trapezoidal integration
@LeahPrice
Copy link
Collaborator Author

@LeahPrice LeahPrice commented Jul 27, 2017

I moved the function stableLogSumWeights to the population header file. It compiled for me and I didn't get any warnings, but I didn't get any warnings before Dirk added the forward declaration either. I think it's okay since Travis CI checks passed.

Some of this is a little different to what I have in my master copy - I changed the template parameter for history to make it easier for users to write the path sampling integrands. I've checked that everything still works properly on the linear regression example but I'm saving that for a future pull request where I'll document it properly. Please let me know if you'd like me to add it in now.

/// normalising weights, calculating the effective sample size and estimating the normalising constant.
///
/// \param logw The log weights of interest.
inline double stableLogSumWeights(const arma::vec & logw){

This comment has been minimized.

@adamjohansen

adamjohansen Jul 27, 2017
Collaborator

I'm doing this too quickly, because I need to check out of my accommodation shortly, but is this "code in a header"? With the exception of /templated/ functions which are instructions to the compiler to generate code rather than code itself, or inline member functions, it's usual to keep only declarations in the header (to prevent the kind of compiler error that Dirk encountered) with the declaration living elsewhere in a source file.

This comment has been minimized.

@LeahPrice

LeahPrice Jul 27, 2017
Author Collaborator

Great, thank you. I've created a new source file to contain that and possible other general use function definitions (it didn't fit naturally under the current source files).


//Normalise the weights
pPopulation.SetLogWeight(pPopulation.GetLogWeight() - dlogNCIt*arma::ones(N));

This comment has been minimized.

@adamjohansen

adamjohansen Jul 27, 2017
Collaborator

Do you actually need to create the vector of ones here (glancing at the Armadillo docs, isn't arma::vec - double well defined)? If you do need the extra vector, it might be sensible to have a static N-vector of ones available to avoid the overheads.

This comment has been minimized.

@LeahPrice

LeahPrice Jul 27, 2017
Author Collaborator

Good point, thanks.

Copy link
Collaborator

@adamjohansen adamjohansen left a comment

Thanks, @LeahPrice, I've made a couple of very small comments but this essentially looks good to me.

and moving the stableLogSumWeights function definition to a source file.
Copy link
Collaborator

@adamjohansen adamjohansen left a comment

Great, this looks good to me.

(I just realised it might not have been obvious from my previous comment, but the reason to avoid introducing the vector of ones is just that it involves allocating N*sizeof(double) bytes of memory and then initializing it every time and N might be quite large.)

@eddelbuettel eddelbuettel merged commit df94c42 into rcppsmc:master Jul 27, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@LeahPrice LeahPrice deleted the LeahPrice:NCestimation branch Jul 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.