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

MCMC Repeats #21

Merged
merged 3 commits into from Aug 8, 2017

Conversation

Projects
None yet
3 participants
@LeahPrice
Collaborator

LeahPrice commented Aug 8, 2017

I accidentally left off an absolute value in one of the calculations for the MCMC repeats. This pull request is focused on

  • fixing that issue
  • storing the MCMC repeats in the history
  • Adding a couple of items to NEWS (about the pull requests so far for adaptation)

Sorry this is a bit of a step backwards in terms of useful pull requests, but I figured it was preferable to do this separately to the next pull request which contains the examples. I have the code for that available here so I will create a pull request for it after this one is sorted.

I had issues with Gitkraken today and ended up having to set up the clone on my desktop again. I don't think that caused any problems.

Can I please get your opinion on the Travis CI note below?

installed size is 6.8Mb
sub-directories of 1Mb or more:
libs 6.5Mb

LeahPrice added some commits Aug 8, 2017

Correcting MCMC Repeats
I managed to forget an absolute value in the previous commit, which led to the incorrect choice of MCMC repeats.
Storing MCMC repeats in history
Adding the number of MCMC repeats to the history element object.
Updating NEWS
Including recent issue tickets and pull requests
@adamjohansen

Looks good to me.

(Just one trivial comment about a use of fabs.)

@@ -118,7 +118,7 @@ namespace smc {
int staticModelAdapt::calcMcmcRepeats(double acceptProb, double desiredAcceptProb, int initialN, int maxReps){
if (acceptProb + 1.0 <= 1e-9){
return initialN;
} else if (acceptProb - 1.0 <= 1e-9){
} else if (fabs(acceptProb - 1.0) <= 1e-9){

This comment has been minimized.

@adamjohansen

adamjohansen Aug 8, 2017

Collaborator

I'm not sure what @eddelbuettel thinks about abs/fabs but I'd be inclined to just use std::abs in C++ code. Not that it is of any consequence... actually,on this instance I would have been inclined to say
if ((acceptProb- 1.0) >= -1e-9) { ...
as there's not much danger of it being larger than 1+1e-9.

@adamjohansen

adamjohansen Aug 8, 2017

Collaborator

I'm not sure what @eddelbuettel thinks about abs/fabs but I'd be inclined to just use std::abs in C++ code. Not that it is of any consequence... actually,on this instance I would have been inclined to say
if ((acceptProb- 1.0) >= -1e-9) { ...
as there's not much danger of it being larger than 1+1e-9.

This comment has been minimized.

@LeahPrice

LeahPrice Aug 8, 2017

Collaborator

Thanks Adam. I'll use that suggestion.

@LeahPrice

LeahPrice Aug 8, 2017

Collaborator

Thanks Adam. I'll use that suggestion.

@adamjohansen

This comment has been minimized.

Show comment
Hide comment
@adamjohansen

adamjohansen Aug 8, 2017

Collaborator

On the size of the library directory, I'd been wondering about the root cause of this and haven't got around to looking yet. The RcppSMC.so file is rather larger considering what it does -- is it possible we're statically linking against something which we shouldn't be?

If it's the size the library needs to be then that's fair enough, but if we're doing something daft that's inflating it to a size rather larger than that of RcppArmadillo.so (which feels likely under the circumstances) then we should certainly address that.

I'll have an investigate later on, but I'd better get on with the day job now.

Collaborator

adamjohansen commented Aug 8, 2017

On the size of the library directory, I'd been wondering about the root cause of this and haven't got around to looking yet. The RcppSMC.so file is rather larger considering what it does -- is it possible we're statically linking against something which we shouldn't be?

If it's the size the library needs to be then that's fair enough, but if we're doing something daft that's inflating it to a size rather larger than that of RcppArmadillo.so (which feels likely under the circumstances) then we should certainly address that.

I'll have an investigate later on, but I'd better get on with the day job now.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Aug 8, 2017

Collaborator

I also prefer std::abs() from C++, but have of course use fabs() in the past.

The "size of the library directory" is a C++ side effect we cannot do anything about, sadly. I get that with many packages.

Collaborator

eddelbuettel commented Aug 8, 2017

I also prefer std::abs() from C++, but have of course use fabs() in the past.

The "size of the library directory" is a C++ side effect we cannot do anything about, sadly. I get that with many packages.

@adamjohansen

This comment has been minimized.

Show comment
Hide comment
@adamjohansen

adamjohansen Aug 8, 2017

Collaborator

Thanks, Dirk, if it's unavoidable I'll stop worrying about it...

Collaborator

adamjohansen commented Aug 8, 2017

Thanks, Dirk, if it's unavoidable I'll stop worrying about it...

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Aug 8, 2017

Collaborator

And friend has played some tricks to get strip to act on it after the build. That, and not using -g, can bring the size down. But both are somewhat non-standard.

Collaborator

eddelbuettel commented Aug 8, 2017

And friend has played some tricks to get strip to act on it after the build. That, and not using -g, can bring the size down. But both are somewhat non-standard.

@eddelbuettel eddelbuettel merged commit 223c585 into rcppsmc:master Aug 8, 2017

1 check passed

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

This comment has been minimized.

Show comment
Hide comment
@LeahPrice

LeahPrice Aug 8, 2017

Collaborator

Thanks Adam and Dirk. I won't worry about the size note too much for now then.

Collaborator

LeahPrice commented Aug 8, 2017

Thanks Adam and Dirk. I won't worry about the size note too much for now then.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Aug 8, 2017

Collaborator

Hi Leah, I see these two uses of abs() and fabs():

~/git/rcppsmc(master)$ ag "abs\(" src/staticModelAdapt.cpp
68:                err = abs(f_m);
121:        } else if (fabs(acceptProb - 1.0) <= 1e-9){
~/git/rcppsmc(master)$ 

If it were I'd be explicit and use std::abs in both cases. Together with cmath.h this should give you double abs(doube) from the std namespace. If the compiler does not complain we probably already use it on the first call.

By the way, it is nice that the compilation proceeds without any warnings. We are at pretty good coding / packaging practices now.

Collaborator

eddelbuettel commented Aug 8, 2017

Hi Leah, I see these two uses of abs() and fabs():

~/git/rcppsmc(master)$ ag "abs\(" src/staticModelAdapt.cpp
68:                err = abs(f_m);
121:        } else if (fabs(acceptProb - 1.0) <= 1e-9){
~/git/rcppsmc(master)$ 

If it were I'd be explicit and use std::abs in both cases. Together with cmath.h this should give you double abs(doube) from the std namespace. If the compiler does not complain we probably already use it on the first call.

By the way, it is nice that the compilation proceeds without any warnings. We are at pretty good coding / packaging practices now.

@LeahPrice

This comment has been minimized.

Show comment
Hide comment
@LeahPrice

LeahPrice Aug 8, 2017

Collaborator

Hi Dirk, for some reason the compiler didn't complain about it but I checked and it wasn't using the double version. Thanks for picking up on that. I'll change the first call to std::abs and the second to Adam's suggestion in the next pull request.

Collaborator

LeahPrice commented Aug 8, 2017

Hi Dirk, for some reason the compiler didn't complain about it but I checked and it wasn't using the double version. Thanks for picking up on that. I'll change the first call to std::abs and the second to Adam's suggestion in the next pull request.

@LeahPrice LeahPrice deleted the LeahPrice:mcmcRepeats branch Aug 12, 2017

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