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

[R-package] use R standard routine to access read-only ints passed to C++ #4246

Merged
merged 6 commits into from
May 3, 2021

Conversation

jameslamb
Copy link
Collaborator

This is another step towards #3016. It proposed using R's standard interface to handle integers passed into the C++ side (for things like array size, buffer size, number of iterations, etc.).

Changes in this PR:

  • use R builtin SEXP type for any arguments passed from R to C++ that should be read-only integers
  • use R builtin Rf_asInteger() to convert those SEXP objects to ints (replacing LightGBM's R_AS_INT)
  • fix minor inconsistencies between lightgbm_R.h and lightgbm_R.cpp (e.g. parameter named nrow in the header file and num_row in the implementation)

Background

There is not a scalar integer type in R. When you run some code like x <- 1L, R creates a length-one array (referred to in R as a "vector").

To treat such data as an integer, instead of an integer array, in C/C++, it's necessary to extract the first element of that array into a new int. R provides a convenience function, Rf_asInteger(), for exactly that purpose.

See https://cran.r-project.org/doc/manuals/r-release/R-exts.html#Some-convenience-functions for more details.

Notes for Reviewers

This does not rely on #4242

@jameslamb
Copy link
Collaborator Author

jameslamb commented May 2, 2021

/gha run r-solaris

Workflow Solaris CRAN check has been triggered! 🚀
https://github.com/microsoft/LightGBM/actions/runs/803383736

solaris-x86-patched: https://builder.r-hub.io/status/lightgbm_3.2.1.99.tar.gz-d3e989e4aa394a59bb7795b6b2bb6718
solaris-x86-patched-ods: https://builder.r-hub.io/status/lightgbm_3.2.1.99.tar.gz-df78c8f2290a4a0d983aed8df559ea56
Reports also have been sent to LightGBM public e-mail: http://www.yopmail.com/lightgbm_rhub_checks
Status: success ✔️.

@jameslamb
Copy link
Collaborator Author

jameslamb commented May 2, 2021

/gha run r-valgrind

Workflow R valgrind tests has been triggered! 🚀
https://github.com/microsoft/LightGBM/actions/runs/803383883

Status: success ✔️.

@StrikerRUS
Copy link
Collaborator

StrikerRUS commented May 2, 2021

/gha run r-valgrind

Workflow R valgrind tests has been triggered! 🚀
https://github.com/microsoft/LightGBM/actions/runs/805263054

Status: success ✔️.

Copy link
Collaborator

@StrikerRUS StrikerRUS left a comment

Choose a reason for hiding this comment

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

I think we can cast buffer_len only once in two places below.

R-package/src/lightgbm_R.cpp Outdated Show resolved Hide resolved
R-package/src/lightgbm_R.cpp Outdated Show resolved Hide resolved
Co-authored-by: Nikita Titov <nekit94-08@mail.ru>
@jameslamb
Copy link
Collaborator Author

jameslamb commented May 3, 2021

/gha run r-valgrind

Workflow R valgrind tests has been triggered! 🚀
https://github.com/microsoft/LightGBM/actions/runs/807172257

Status: success ✔️.

@jameslamb
Copy link
Collaborator Author

I think we can cast buffer_len only once in two places below.

nice! valgrind seems happy with that change (#4246 (comment)) and so does the rest of CI, so I'll merge this.

@jameslamb jameslamb merged commit aedfdd0 into microsoft:master May 3, 2021
@jameslamb jameslamb deleted the r/get-ints branch May 3, 2021 16:10
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity since it was closed. To start a new related discussion, open a new issue at https://github.com/microsoft/LightGBM/issues including a reference to this.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants