search

kosukeimai / CBPS

R package: CBPS
27 stars 10 forks source link

Enhancement to the package: documentation by Roxygen2; testthat added #9

Closed HJ08003 closed 7 years ago

HJ08003 commented 7 years ago

(1) Added Roxygen2 comments in .R to automatically generate .Rd and NAMESPACE. (2) Added testthat to automatically check for errors. (3) Revised the code to eliminate some warnings (NA is added in the following).

X <- if (!is.empty.model(mt)) model.matrix(mt, mf)#[,-2] else matrix(NA, NROW(Y), 0L)

christianfong commented 7 years ago

The LaLonde.Rd conflict is just a typo correction because CRAN check was complaining. Go ahead and use the new one.

Also, I made a couple of bug fixes to the code about a week ago, particularly fixing an issue with the sample weights in CBPS Binary. Just to confirm, those have survived the proposed merge, right?

kosukeimai commented 7 years ago

@HJ08003 Please resolve the conflict

HJ08003 commented 7 years ago

@christianfong I will update the CBPS-package.R for that typo on my side and regenerate the LaLonde.Rd. For the changes you made in CBPSBinary.R that was committed to the master branch 8 days ago, I will replace mine with that most recent copy.

HJ08003 commented 7 years ago

I think it is ready to be merged: (1) CBPS-package.R has been revised and LaLonde.Rd generated with the typo corrected. (2) Used your updated CBPSBinary.R (3) Test cases updated accordingly, after using the new CBPSBinary.

The package has passed the following check: (1) R CMD check --as-cran (2) travis-ci (3) Win-builder

christianfong commented 7 years ago

Wonderful. Sounds ready to me too. I promised Noah Greifer that I'd add a reference to cobalt in the documentation, but I can take care of that after the merge. After that, I think we're ready to push an update to CRAN.

christianfong commented 7 years ago

Wonderful. Sounds ready to me too. I promised Noah Greifer that I'd add a reference to cobalt in the documentation, but I can take care of that after the merge. After that, I think we're ready to push an update to CRAN.

kosukeimai commented 7 years ago

@christianfong Please merge this if it looks good to you. Thanks.

HJ08003 commented 7 years ago

@christianfong Please take a look at the .Rd files --- what I did was reverse engineering to automatically insert Roxygen2 comments in the .R files which were used to generate the *.Rd files.

christianfong commented 7 years ago

Is this on a separate branch somewhere? I am trying to figure out how I can review the changes before agreeing to a merge, but all of the stuff I can find on Google talks about checking out the branch to be merged.

HJ08003 commented 7 years ago

You may clone a copy from my branch: git clone https://github.com/HJ08003/CBPS

kosukeimai commented 7 years ago

@christianfong Have you tried: https://github.com/kosukeimai/CBPS/pull/9/files

christianfong commented 7 years ago

Looks great to me! The only thing is that the alias fields in the documentation seem to be off, but I see no reason why that should keep us from merging. We can take care of the alias issue on master.