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

Header-only patch #29

Merged
merged 2 commits into from Jan 24, 2018

Conversation

Projects
None yet
3 participants
@mlysy
Contributor

mlysy commented Jan 24, 2018

Dear RcppSMC developers,

Love the library. I tried to use it in an R/C++ package through the //[[Rcpp::depends(RcppSMC)]] mechanism, but the compiler couldn't find a couple of cpp files in src that (presumably) should have been in inst/include. This patch relocates the contents of these files while keeping things headers-only.

@eddelbuettel

This comment has been minimized.

Collaborator

eddelbuettel commented Jan 24, 2018

Interesting :) @adamjohansen and I will review.

I just noticed that Travis may not have been "on" (my mistake) since we moved this into its own org. Did you have a Travis test for this?

@eddelbuettel

This comment has been minimized.

Collaborator

eddelbuettel commented Jan 24, 2018

Looks good locally

edd@rob:~/git/rcppsmc(master)$ git pu
Fetching origin
Already up-to-date.
edd@rob:~/git/rcppsmc(master)$ git checkout -b feature/pr29
Switched to a new branch 'feature/pr29'
edd@rob:~/git/rcppsmc(feature/pr29)$ git pull git://github.com/mlysy/rcppsmc.git header-only-patch
remote: Counting objects: 17, done.
remote: Total 17 (delta 13), reused 13 (delta 13), pack-reused 4
Unpacking objects: 100% (17/17), done.
From git://github.com/mlysy/rcppsmc
 * branch            header-only-patch -> FETCH_HEAD
Updating 8ca9ad4..fe28977
Fast-forward
 .gitignore                               |  1 +
 {src => deprecated}/helper.cpp           |  0
 {src => deprecated}/history.cpp          |  0
 {src => deprecated}/smc-exception.cpp    |  0
 {src => deprecated}/staticModelAdapt.cpp |  0
 inst/include/history.h                   | 16 ++++++++++++++--
 inst/include/population.h                | 10 +++++++++-
 inst/include/sampler.h                   |  2 +-
 inst/include/smc-exception.h             | 35 ++++++++++++++++++++++++++++++-----
 inst/include/staticModelAdapt.h          | 93 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 10 files changed, 148 insertions(+), 9 deletions(-)
 rename {src => deprecated}/helper.cpp (100%)
 rename {src => deprecated}/history.cpp (100%)
 rename {src => deprecated}/smc-exception.cpp (100%)
 rename {src => deprecated}/staticModelAdapt.cpp (100%)
edd@rob:~/git/rcppsmc(feature/pr29)$ build.r 
* checking for file ‘./DESCRIPTION’ ... OK
* preparing ‘RcppSMC’:
* checking DESCRIPTION meta-information ... OK
* cleaning src
* running ‘cleanup’
* checking for LF line-endings in source and make files and shell scripts
* checking for empty or unneeded directories
* looking to see if a ‘data/datalist’ file should be added
* building ‘RcppSMC_0.2.0.1.tar.gz’

edd@rob:~/git/rcppsmc(feature/pr29)$ rcc.r RcppSMC_0.2.0.1.tar.gz                                        
────────────────────────────────────────────────────────────────────────────────                         
─  using log directory ‘/tmp/file31c359c18cdd/RcppSMC.Rcheck’                                            
─  using R version 3.4.3 (2017-11-30)               
─  using platform: x86_64-pc-linux-gnu (64-bit)     
─  using session charset: UTF-8                     
✔  checking for file ‘RcppSMC/DESCRIPTION’          
─  checking extension type ... Package              
─  this is package ‘RcppSMC’ version ‘0.2.0.1’      
✔  checking package namespace information           
✔  checking package dependencies                    
✔  checking if this is a source package             
✔  checking if there is a namespace                 
✔  checking for executable files                    
✔  checking for hidden files and directories        
✔  checking for portable file names                 
✔  checking for sufficient/correct file permissions 
─  checking whether package ‘RcppSMC’ can be installed ... [48s/45s] OK
✔  checking installed package size
✔  checking package directory
✔  checking DESCRIPTION meta-information
✔  checking top-level files
✔  checking for left-over files
✔  checking index information
✔  checking package subdirectories
✔  checking R files for non-ASCII characters
✔  checking R files for syntax errors
✔  checking whether the package can be loaded
✔  checking whether the package can be loaded with stated dependencies
✔  checking whether the package can be unloaded cleanly
✔  checking whether the namespace can be loaded with stated dependencies
✔  checking whether the namespace can be unloaded cleanly
✔  checking loading without being on the library search path
✔  checking loading without being on the library search path
✔  checking dependencies in R code
✔  checking S3 generic/method consistency
✔  checking replacement functions
✔  checking foreign function calls
✔  checking R code for possible problems
✔  checking Rd files
✔  checking Rd metadata
✔  checking Rd cross-references
✔  checking for missing documentation entries
✔  checking for code/documentation mismatches
✔  checking Rd \usage sections
✔  checking Rd contents
✔  checking for unstated dependencies in examples
✔  checking contents of ‘data’ directory
✔  checking data for non-ASCII characters
✔  checking data for ASCII and uncompressed saves
✔  checking line endings in shell scripts
✔  checking line endings in C/C++/Fortran sources/headers
✔  checking line endings in Makefiles
✔  checking compilation flags in Makevars
✔  checking for GNU extensions in Makefiles
✔  checking for portable use of $(BLAS_LIBS) and $(LAPACK_LIBS)
✔  checking compiled code
✔  checking examples
✔  checking PDF version of manual

── 0 errors ✔ | 0 warnings ✔ | 0 notes ✔
edd@rob:~/git/rcppsmc(feature/pr29)$ 

Looks good.

@eddelbuettel

This comment has been minimized.

Collaborator

eddelbuettel commented Jan 24, 2018

Would you do us the favour and reveal a little more than mlysy <mlysy@users.noreply.github.com> ?

You know who we are.

@adamjohansen

This comment has been minimized.

Collaborator

adamjohansen commented Jan 24, 2018

Looks good to me too:

adam@localhost ~/repo/rcppsmc $ git checkout -b feature/pr29
Switched to a new branch 'feature/pr29'
adam@localhost ~/repo/rcppsmc $ git pull git://github.com/mlysy/rcppsmc.git header-only-patch
remote: Counting objects: 17, done.
remote: Total 17 (delta 13), reused 13 (delta 13), pack-reused 4
Unpacking objects: 100% (17/17), done.
From git://github.com/mlysy/rcppsmc
 * branch            header-only-patch -> FETCH_HEAD
Updating 8ca9ad4..fe28977
Fast-forward
 .gitignore                               |  1 +
 {src => deprecated}/helper.cpp           |  0
 {src => deprecated}/history.cpp          |  0
 {src => deprecated}/smc-exception.cpp    |  0
 {src => deprecated}/staticModelAdapt.cpp |  0
 inst/include/history.h                   | 16 +++++-
 inst/include/population.h                | 10 +++-
 inst/include/sampler.h                   |  2 +-
 inst/include/smc-exception.h             | 35 ++++++++++--
 inst/include/staticModelAdapt.h          | 93 ++++++++++++++++++++++++++++++++
 10 files changed, 148 insertions(+), 9 deletions(-)
 rename {src => deprecated}/helper.cpp (100%)
 rename {src => deprecated}/history.cpp (100%)
 rename {src => deprecated}/smc-exception.cpp (100%)
 rename {src => deprecated}/staticModelAdapt.cpp (100%)
adam@localhost ~/repo/rcppsmc $ cd ..
adam@localhost ~/repo $ R CMD build rcppsmc/
* checking for file 'rcppsmc/DESCRIPTION' ... OK
* preparing 'RcppSMC':
* checking DESCRIPTION meta-information ... OK
* cleaning src
* running 'cleanup'
* checking for LF line-endings in source and make files
* checking for empty or unneeded directories
* looking to see if a 'data/datalist' file should be added
* building 'RcppSMC_0.2.0.1.tar.gz'
 R CMD check RcppSMC_0.2.0.1.tar.gz 
* using log directory '/home/adam/repo/RcppSMC.Rcheck'
* using R version 3.4.1 (2017-06-30)
* using platform: x86_64-pc-linux-gnu (64-bit)
* using session charset: ISO8859-1
* checking for file 'RcppSMC/DESCRIPTION' ... OK
* checking extension type ... Package
* this is package 'RcppSMC' version '0.2.0.1'
* checking package namespace information ... OK
* checking package dependencies ... OK
* checking if this is a source package ... OK
* checking if there is a namespace ... OK
* checking for executable files ... OK
* checking for hidden files and directories ... OK
* checking for portable file names ... OK
* checking for sufficient/correct file permissions ... OK
* checking whether package 'RcppSMC' can be installed ...
* checking installed package size ... OK
* checking package directory ... OK
* checking DESCRIPTION meta-information ... OK
* checking top-level files ... OK
* checking for left-over files ... OK
* checking index information ... OK
* checking package subdirectories ... OK
* checking R files for non-ASCII characters ... OK
* checking R files for syntax errors ... OK
* checking whether the package can be loaded ... OK
* checking whether the package can be loaded with stated dependencies ... OK
* checking whether the package can be unloaded cleanly ... OK
* checking whether the namespace can be loaded with stated dependencies ... OK
* checking whether the namespace can be unloaded cleanly ... OK
* checking loading without being on the library search path ... OK
* checking dependencies in R code ... OK
* checking S3 generic/method consistency ... OK
* checking replacement functions ... OK
* checking foreign function calls ... OK
* checking R code for possible problems ... OK
* checking Rd files ... OK
* checking Rd metadata ... OK
* checking Rd cross-references ... OK
* checking for missing documentation entries ... OK
* checking for code/documentation mismatches ... OK
* checking Rd \usage sections ... OK
* checking Rd contents ... OK
* checking for unstated dependencies in examples ... OK
* checking contents of 'data' directory ... OK
* checking data for non-ASCII characters ... OK
* checking data for ASCII and uncompressed saves ... OK
* checking line endings in C/C++/Fortran sources/headers ... OK
* checking line endings in Makefiles ... OK
* checking compilation flags in Makevars ... OK
* checking for GNU extensions in Makefiles ... OK
* checking for portable use of $(BLAS_LIBS) and $(LAPACK_LIBS) ... OK
* checking compiled code ... OK
* checking examples ... OK
* checking PDF version of manual ... OK
* DONE

Status: OK

I'll have a proper look later... but it looks pretty uncontroversial. Thanks for the fix.

@adamjohansen

There's the philosophical question of how far one is prepared to go in squeezing code into headers in order to avoid some linking.

I don't think this PR goes too far, and I can see advantages so I'm fine with this.

Thanks to the mysterious @mlysy.

double dMaxWeight = arma::max(logw);
double sum = arma::sum(exp(logw - dMaxWeight));
return (dMaxWeight + log(sum));
}

This comment has been minimized.

@adamjohansen

adamjohansen Jan 24, 2018

Collaborator

I have a vague aesthetic objection to non-templated code in header files, but that's because I'm a dinosaur. With inline functions I guess it doesn't matter anyway, so I don't have a problem with it if @eddelbuettel is happy. I can see at least small advantages in keeping the library side of the code header only if it doesn't cause other problems.

@@ -76,5 +76,98 @@ namespace smc {
/// Returns the empirical covariance matrix based on the current weighted particle set.
arma::mat GetEmpCov(void) const {return empCov;}
};

This comment has been minimized.

@adamjohansen

adamjohansen Jan 24, 2018

Collaborator

Again, you could debate whether all of what follows belongs inline in headers; much of this is prototypical code that a user would need to replicate to implement a different adaptation scheme, but again I don't feel particularly strongly either way.

@eddelbuettel

This comment has been minimized.

Collaborator

eddelbuettel commented Jan 24, 2018

Yes, people also debate longer compile times (true) versus the fact that here we would have it much easier to let others use this just via headers (RcppArmadillo-style). I tend to side with "headers-only is good" for the easier use case. but try not to be dogmatic.

Would still be nice to know who we got this from. Could you email Adam and/or myself what name we should use for you were we to merge this?

@mlysy

This comment has been minimized.

Contributor

mlysy commented Jan 24, 2018

Dear package developers,

Oh my...did not mean to come off as mysterious. My name is Martin Lysy. I'm very glad you're considering the merge. I am using RcppSMC to implement a PMCMC algorithm for stochastic differential equations in the R package msde. I would be most pleased if useRs could simply download my package from CRAN with all required dependencies, as opposed to e.g., me directing them to an RcppSMC fork...

@eddelbuettel

This comment has been minimized.

Collaborator

eddelbuettel commented Jan 24, 2018

Thanks, Martin, much appreciated. I am following a GNU convention of logging changes in a file ChangeLog with a standard "Date Name Email" header of each paragraph (and Emacs automates this). You git commit had no traces to a real name, or email which is why I asked. I'll fill in based on msde if that is ok.

And I with you: LinkingTo etc are marvelous and make deployment much easier so the move to a header-only package is entirely defensible. And now that you did the work, I'll merge and do the cleanup (including my own snafu of disconnecting README.md from the Travis CI status).

@eddelbuettel eddelbuettel merged commit ec97a29 into rcppsmc:master Jan 24, 2018

@adamjohansen

This comment has been minimized.

Collaborator

adamjohansen commented Jan 24, 2018

Yes, thanks Martin... I wasn't trying to be difficult and what you've done certainly makes sense. Good code's always welcome, and much appreciated.

@mlysy

This comment has been minimized.

Contributor

mlysy commented Jan 25, 2018

Thanks guys! I really enjoyed looking through this library and studying some of your programming tricks. I hope you don't mind if I contact you down the road with some other feature/pull requests which hopefully aren't purely self-serving...I'll follow Dirk's advice to start with an "issue" first to get your feedback on whether/how to proceed.

@mlysy mlysy deleted the mlysy:header-only-patch branch Jan 25, 2018

@eddelbuettel

This comment has been minimized.

Collaborator

eddelbuettel commented Jan 25, 2018

Don't run away we still need you :) I started riffing on the commuter train this morning (but my ride is short) about making a "plugin" so that we can do // [[Rcpp::depends(RcppSMC)]]. Didn't quite get there but I presume you have a test case?

Also, as you're in Waterlook, say Hi to Marius Hofert ...

@mlysy

This comment has been minimized.

Contributor

mlysy commented Jan 26, 2018

I'm happy to make one and send it over.

One issue I did neglect though is that since the smctc header file now does #include <RcppArmadillo.h>, this means anyone including the library now has to change all instances of #include <Rcpp.h> to #include <RcppArmadillo.h>...

How do you guys feel about this? To my recollection the dependence on RcppArmadillo is very minor, and I'd be happy to rewrite it using "plain" C++...

@eddelbuettel

This comment has been minimized.

Collaborator

eddelbuettel commented Jan 26, 2018

No, we added Armadillo on purpose last summer. Also, RcppArmadillo.h does in fact pull in Rcpp.h for you (in the right order) so by saying #include <RcppSMC.h> you now get all three "for the price of one".

Which strikes me as a good thing.

@mlysy

This comment has been minimized.

Contributor

mlysy commented Jan 27, 2018

I have no issue with RcppArmadillo.h being included, it's rather that if, say, I had another R package with lots of #include <Rcpp.h> to which I now want to add a feature with RcppSMC, then I need to change all of those includes to RcppArmadillo.h (or RcppSMC), otherwise compiler complains. Doesn't this seem like a something supposed to be at the bottom of the dependency tree (i.e. Armadillo) jumping up to the top?

@mlysy

This comment has been minimized.

Contributor

mlysy commented Jan 27, 2018

OK I wrote a simple test case for the Rcpp::depends mechanism:

  • Created pfLineart_honly.cpp and pfLineartBS_honly.R with minuscule modifications to pfLineart.cpp and pfLineartBS_honly.R. Basically pfLineart_honly.cpp now starts with:
//[[Rcpp::depends("RcppArmadillo")]]
//[[Rcpp::depends("RcppSMC")]]
  • The R test script is then:
# compile on-the-fly with Rcpp::depends("RcppSMC")
Rcpp::sourceCpp(file = "pflineart_honly.cpp")
source("pfLineartBS_honly.R") # the R wrapper

# compare to RcppSMC version
require(RcppSMC)
linData <- simLineart() # simulate data
set.seed(1043)
pf1 <- pfLineartBS(data = linData$data, plot = FALSE) # with RcppSMC
set.seed(1043)
pf2 <- pfLineartBS_honly(data = linData$data, plot = FALSE) # with Rcpp::depends
identical(pf1, pf2) # should be the same

Please let me know whether/how I should incorporate these files into the package. I know Dirk is not too keen on sourceCpp inside R tests & examples...

@eddelbuettel

This comment has been minimized.

Collaborator

eddelbuettel commented Jan 27, 2018

sourceCpp() inside a file can be perfectly normal -- that is what we do eg for tests

For the package main you don't need it.

The plugin use we have is simply in addtion to the standard package use we have.

@mlysy mlysy referenced this pull request Jan 27, 2018

Merged

initial plugin support #30

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