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

PUBDEV-7657: link proper base R manuals in R h2o manual #4765

Merged
merged 6 commits into from
Sep 3, 2020
Merged

Conversation

jangorecki
Copy link
Contributor

@jangorecki jangorecki commented Jul 2, 2020

https://0xdata.atlassian.net/browse/PUBDEV-7657

Can be tested on R-devel image using _R_CHECK_XREFS_MIND_SUSPECT_ANCHORS_=true env var.
Below example assumes repos generated by this PR. PR does not produce R repo one needs to put package sources manually in current directory.

docker pull registry.gitlab.com/jangorecki/dockerfiles/r-devel
docker run -it registry.gitlab.com/jangorecki/dockerfiles/r-devel /bin/bash
R -q -e 'install.packages(c("RCurl","jsonlite"))'
R -q -e 'download.packages("h2o", ".", repos="https://h2o-release.s3.amazonaws.com/h2o/PUBDEV-7657/5118/R")'
#no package ‘h2o’ at the repositories
export _R_CHECK_XREFS_MIND_SUSPECT_ANCHORS_=true
export _R_CHECK_FORCE_SUGGESTS_=false
R CMD check --ignore-vignettes --as-cran h2o_3.31.0.5118.tar.gz

And look for checking Rd cross-references line.


Moreover I think there is space for improvement, for example on merging manual pages. It is not really reasonable to have log, log10, log1p, log2 each having own manual page. IMO it would be easier to write nice documentation when not trying to fit into roxygen templates.

@jangorecki jangorecki requested a review from ledell July 2, 2020 16:36
@jangorecki jangorecki changed the title link proper base R manuals in R h2o manual PUBDEV-7657: link proper base R manuals in R h2o manual Jul 2, 2020
@jangorecki jangorecki added the R label Jul 2, 2020
@ledell
Copy link
Contributor

ledell commented Jul 3, 2020

Thanks @jangorecki! Can you change this to merge into rel-zahradnik instead? We need to get this fixed on the current release. I think it shouldn't have any conflicts...

Copy link
Contributor

@ledell ledell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @jangorecki I don't know if there was some find/replace thing that went wrong here, but it seems like you're changing a bunch of functions to something else (related to original function). e.g. rowMeans -> colSums, var -> cor,... I am not sure what's going on. I started to make suggested changes on each one, but since there's a lot of them, I though I'd summarize my feedback here.

There's also changes liek as.factor -> factor, as.numeric -> numeric. Not sure what this is for either.

I actually don't see any of the affected functions which were listed in the JIRA ticket: https://0xdata.atlassian.net/browse/PUBDEV-7657

h2o-r/h2o-package/R/frame.R Outdated Show resolved Hide resolved
@@ -2710,7 +2710,7 @@ median.H2OFrame <- h2o.median
#' is ignored.
#' @param return_frame \code{logical}. Indicate whether to return an H2O frame or a list. Default is FALSE (returns a list).
#' @param ... Further arguments to be passed from or to other methods.
#' @seealso \code{\link[base]{mean}} , \code{\link[base]{rowMeans}}, or \code{\link[base]{colMeans}} for the base R implementation
#' @seealso \code{\link[base]{mean}} , \code{\link[base]{colSums}}, or \code{\link[base]{colMeans}} for the base R implementation
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#' @seealso \code{\link[base]{mean}} , \code{\link[base]{colSums}}, or \code{\link[base]{colMeans}} for the base R implementation
#' @seealso \code{\link[base]{mean}} , \code{\link[base]{rowMeans}}, or \code{\link[base]{colMeans}} for the base R implementation

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jangorecki Do you know why this was changed from rowMeans to colSums?

h2o-r/h2o-package/R/frame.R Outdated Show resolved Hide resolved
@jangorecki
Copy link
Contributor Author

jangorecki commented Jul 3, 2020

I followed WRE and advice on the mailing list about addressing this change. Unfortunately I don't have an environment on hand to build h2o package from this branch, so cannot fully test that.

@jangorecki
Copy link
Contributor Author

jangorecki commented Jul 3, 2020

Testing revealed one more missing entry to fix.
Tested on the 5118 master after applying fix from this branch:

docker pull registry.gitlab.com/jangorecki/dockerfiles/r-devel
docker run -it registry.gitlab.com/jangorecki/dockerfiles/r-devel /bin/bash
apt-get -qq update
apt-get install -y libxml2-dev
R -q -e 'install.packages(c("RCurl","jsonlite","roxygen2"))'
R -q -e 'download.packages("h2o", ".", repos="https://h2o-release.s3.amazonaws.com/h2o/master/5118/R")'
tar xvf h2o_3.31.0.5118.tar.gz
mv h2o_3.31.0.5118.tar.gz h2o_3.31.0.5118.tar.gz_old
cd h2o
git init
wget https://gist.githubusercontent.com/jangorecki/9884a919188c06b320bc3231990f3cf4/raw/0adeec6b8f184db0a0005241a053d016104b0f60/h2o-patch.diff ## this does not have one missed topic from recent commit
sed -i 's+h2o-r/h2o-package/R+R+g' h2o-patch.diff
git apply h2o-patch.diff
cd ..
R -q -e 'roxygen2::roxygenize("h2o")'
R CMD build --no-build-vignettes h2o
export _R_CHECK_XREFS_MIND_SUSPECT_ANCHORS_=true
export _R_CHECK_FORCE_SUGGESTS_=false
R CMD check --ignore-vignettes --as-cran h2o_3.31.0.5118.tar.gz

@ledell
Copy link
Contributor

ledell commented Jul 6, 2020

@jangorecki Please see my comments here and let me know what you think.

You said you don't have an environment where you can test this branch... would you be able to build the h2o.tar.qz file from rel-zahradnik with the updated Roxygen2 installed (locally), and then test it if passes the CRAN check on r-devel just by using R-hub?

In order for me to get the latest version up on CRAN, this has to be fixed on rel-zahradnik. Once everything looks good, then I was planning on manually patching the R files inside the existing h2o_3.30.0.6.tar.gz release because it will be a while until 3.30.0.7 comes out and this needs to be up on CRAN before Friday, July 10 (useR! 2020). Since we are not going to release 3.32.0.1 from master until September at the earliest, having these changes only on master is not ideal. I guess the alternative would be to merge to master and then cherry pick that to rel-zahradnik but that seems only complicated if we can just go directly to rel-zahradnik.

@jangorecki
Copy link
Contributor Author

Environment for testing this change is in my previous post. Only replace h2o package there by changing master to required branch.
As for merging into other branch, you can checkout this branch into exactly same branch but new name, push it to GH and open PR against another branch. Should be easier than cherry picking. I don't have workstation on hand already.

@ledell ledell changed the base branch from master to rel-zeno September 3, 2020 06:43
h2o-r/h2o-package/R/frame.R Outdated Show resolved Hide resolved
h2o-r/h2o-package/R/frame.R Outdated Show resolved Hide resolved
h2o-r/h2o-package/R/frame.R Outdated Show resolved Hide resolved
Copy link
Contributor

@ledell ledell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jangorecki I added some extra info (I put the corresponding R base function name in \code{} to not lose context). This is ready to go!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants