-
Notifications
You must be signed in to change notification settings - Fork 13
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
Commonly used adaptation methods #20
Conversation
Adding a class containing commonly used parameters and methods to adapt them (for static Bayesian models).
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.
Nice -- you are clearly getting the drift of incremental value-added PRs. We're in a good spot now.
I'll wait for Adam to peek at this tomorrow though before I merge.
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 looks good to me.
There are a few small things it might make sense to tweak, particularly in the acceptProb comparisons (not that I'd expect these to have any significant impact).
src/staticModelAdapt.cpp
Outdated
/// \param desiredCESS The target conditional ESS for the next temperature (generally fixed). | ||
void staticModelAdapt::ChooseTemp(const arma::vec & logweight, const arma::vec & loglike, double desiredCESS) { | ||
double temp_curr = temp.back(); | ||
if (CESSdiff(logweight,loglike,1-temp_curr,desiredCESS)>=0){ |
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.
Might it make sense for the comparison to be with -0.01 (or the tolerance) to be consistent with the inner workings of the bisection function?
src/staticModelAdapt.cpp
Outdated
m = (a+b)/2.0; | ||
f_m = CESSdiff(logweight,loglike,m-curr,desiredCESS); | ||
err = 10; | ||
while (err > 0.01){ |
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 might be good if it were possible to choose this tolerance without changing a hard-coded constant. In some applications the desired CESS might be larger than 0.99 and a tolerance of 0.01 would be problematic in those settings.
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.
Thanks Adam for this and for all of your other good points. I should have picked up on at least some of these myself -- sorry about that. I've changed this and a couple of other minor things (like writing 1.0 instead of 1 if it's a double) and will do another commit when the check finishes.
/// \param desiredAcceptProb The desired probability of a successful move for each particle. | ||
/// \param initialN The initial number of MCMC repeats. | ||
/// \param maxReps The maximum number of MCMC repeats. | ||
int staticModelAdapt::calcMcmcRepeats(double acceptProb, double desiredAcceptProb, int initialN, int maxReps){ |
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's a bad idea to compare doubles with integers using ==, usually (the finite precision issue), might it make sense to replace these comparisons with
abs(acceptProb - 1) <= 10**-9
or similar?
Great, thanks. |
Adding a class containing commonly used parameters and methods to adapt them (for static Bayesian models).