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

Fixed misc CRAN issues #1260

Merged
merged 2 commits into from
Mar 16, 2018
Merged

Fixed misc CRAN issues #1260

merged 2 commits into from
Mar 16, 2018

Conversation

jameslamb
Copy link
Collaborator

Hello,

Thank you for this excellent package! @randxie and I would like to help you address #629 . We have some experience on the team working with the wonderful eccentricities of getting accepted to CRAN.

What I changed

Please consider this first PR to address some low-hanging notes and warnings. I've also attached a file with the state of the R CMD CHECK output showing the remaining notes and warnings that could be addressed in follow-up PRs.

r_cmd_check.log

As of this PR, R CMD CHECK for this package is down to 4 warnings and 3 notes.

Setup Information

I built the package by running the following from the repo root (I am on MacOS):

# Avoid errors related to AppleClang
export CXX=g++-7 CC=gcc-7

# Build some artifacts and document
Rscript -e "devtools::document()"

# Build the package tarball
cd R-package && Rscript build_package.R

# re-running documentation
Rscript -e "devtools::document()"

# Run R CMD CHECK and save results to file
R CMD CHECK lightgbm_2.1.0.tar.gz --as-cran --no-tests | tee r_cmd_check.log | cat

Please let me know if you have any questions or concerns! Please also let @randxie and I know if you are open to us making additional contributions to move this package toward CRAN-readiness.

Thanks!

-James

@msftclas
Copy link

msftclas commented Mar 13, 2018

CLA assistant check
All CLA requirements met.

@@ -5,7 +5,7 @@ Version: 2.1.0
Date: 2018-01-25
Author: Guolin Ke <guolin.ke@microsoft.com>
Maintainer: Guolin Ke <guolin.ke@microsoft.com>
Description: LightGBM is a gradient boosting framework that uses tree based learning algorithms.
Description: Tree based algorithms can be improved by introducing boosting frameworks. LightGBM is one such framework.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is in response to my least-favorite CRAN note. Your package's Description cannot start with the words "this package" or the name of the package.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@jameslamb I think this Description is too short. We can copy some from https://github.com/Microsoft/LightGBM/blob/master/README.md

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@guolinke would you like me to make that change on this PR?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah

#' Main CV logic for LightGBM
#'
#' @title Main CV logic for LightGBM
#' @name lgb.cv
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this and lgb.train were getting grouped into a single documentation object with interwoven fields and R CMD CHECK complained about duplicated parameters. Using explicit @name elements avoids this

@@ -122,5 +122,28 @@ NULL
# Various imports
#' @import methods
#' @importFrom R6 R6Class
#' @useDynLib lightgbm
#' @useDynLib lib_lightgbm
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was unable to build the package without making this change


# Suppress false positive warnings from R CMD CHECK about
# "unrecognized global variable"
globalVariables(c(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This suppresses the R CMD CHECK warning about "unrecognized global variables"

@@ -0,0 +1 @@

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

these address warnings from CRAN about having Make scripts with only LF characters in them

@guolinke
Copy link
Collaborator

ping @Laurae2

@Laurae2 Laurae2 requested a review from guolinke March 13, 2018 22:47
@Laurae2
Copy link
Contributor

Laurae2 commented Mar 13, 2018

@guolinke Please review the R-package description if you need to make changes on it.

You can compare with xgboost: https://github.com/dmlc/xgboost/blob/master/R-package/DESCRIPTION

@chivee
Copy link
Collaborator

chivee commented Mar 14, 2018

@guolinke , adding @jameslamb and @Laurae2 to the author list will be nice.

@jameslamb , could you please also sign the CLA?

@jameslamb
Copy link
Collaborator Author

@chivee sorry for the delay. CLA signed!

@Laurae2 Laurae2 mentioned this pull request Mar 14, 2018
23 tasks
It is designed to be distributed and efficient with the following advantages:
1. Faster training speed and higher efficiency.
2. Lower memory usage.
3. Better accuracy.
4. Parallel learning supported.
5. Capable of handling large-scale data.
In recognition of these advantages, LightGBM has being widely-used in many winning solutions of machine learning competitions.

Comparison experiments on public datasets suggest that LightGBM can outperform existing boosting frameworks on both efficiency and accuracy, with significantly lower memory consumption. In addition, parallel experiments suggest that in certain circumstances, LightGBM can achieve a linear speed-up in training time by using multiple machines.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@guolinke per your request, I've included additional details here. You will recognize most of this from the README you linked me too. I also added the line about "this is the R interface to the project" following the way XGBoost structured their DESCRIPTION: https://cran.r-project.org/web/packages/xgboost/index.html

I think that's a good clarification for people, to say that LightGBM is a framework and this is the interface to it implemented in a particular programming language.

@guolinke guolinke merged commit 6bb61ea into microsoft:master Mar 16, 2018
@guolinke
Copy link
Collaborator

@Laurae2 can you create another PR to add yourself and other R's contributors to author List ?

@Laurae2
Copy link
Contributor

Laurae2 commented Mar 18, 2018

@guolinke Yes I'll do when I get time tonight or next week.

Do I add both @jameslamb and @yanyachen ?

Laurae2 added a commit to Laurae2/LightGBM that referenced this pull request Mar 24, 2018
guolinke pushed a commit that referenced this pull request Mar 25, 2018
* Add authors #1260

* Add line break at the end of file
@Jorgfevi
Copy link

Jorgfevi commented Jan 8, 2019

Hola , que tal amigos
Tengo el archivo .exe y el dll , pero a la hora de ejecutar o tratar de instalar la libreria R que dice build_r.R me sale un error como este
installation of package ‘C:/Users/BP2499/Documents/R/R-3.5.2/LightGbm/lightgbm_2.2.3.tar.gz’ had non-zero exit status
Desearia saber a que se debe este problema , o que pasos se siguen despues de tener el archivo .exe y .dll
Gracias

@lock lock bot locked as resolved and limited conversation to collaborators Mar 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants