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 routines to access numeric and integer array data in C++ #4247

Merged
merged 6 commits into from
May 3, 2021

Conversation

jameslamb
Copy link
Collaborator

@jameslamb jameslamb commented May 2, 2021

Another step towards #3016.

Fixes #4216.

Changes in this PR:

  • removes LightGBM-custom pointers to R data (R_REAL_PTR(), R_INT_PTR()) with the standard equivalents from Rinternals.h (REAL(), INTEGER())
  • converts arguments on the C++ side that are accessed that way from LGBM_SE to SEXP

Description

R is a dynamically-typed language. You can run code like x <- c(1, 2, 3) without declaring that x is a numeric array, and R will just figure it out.

R makes this possible by storing data for an object in a structure called a SEXPREC. That structure will contain different data based on the R class (integer, character, function, etc.). Libraries can reference that data in C code using a type, SEXP, provided by Rinternals.h. Libraries can also get a pointer to a particular type of data within a SEXPREC using other functions provided by Rinternals.h, for example REAL() to get a pointer to its numeric data.

As described in #4216, the internal details of the SEXPREC struct can change between different versions of R. SEXP is the official R API into that struct, and by using it libraries can reliably stay compatible with multiple R versions.

This PR's changes fix a bug that currently exists when using {lightgbm} with R 3.6, and protects {lightgbm} from similar bugs in the future.

For more details on this, see https://cran.r-project.org/doc/manuals/r-release/R-ints.html#SEXPs.

Notes for Reviewers

This PR does not depend on #4242 or #4247

expect_true(abs(mae - expected_mae) < TOLERANCE)
}
})

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks to @david-cortes for providing the write-up in #4216, which included the reproducible example that inspired this unit test.

I tested this unit test tonight on my Mac, with R 3.6 and {lightgbm} 3.2.1 installed from CRAN, and confirmed that it failed in exactly the way described in #4216. So I'm confident that this test passing means the changes in this PR fix #4216.

@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/803646649

solaris-x86-patched: https://builder.r-hub.io/status/lightgbm_3.2.1.99.tar.gz-c0227bf93bb84b58be157cf6186f1119
solaris-x86-patched-ods: https://builder.r-hub.io/status/lightgbm_3.2.1.99.tar.gz-d66322e8d51b41f3a298625253478da5
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/803647172

Status: success ✔️.

@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/807454258

Status: success ✔️.

@jameslamb
Copy link
Collaborator Author

jameslamb commented May 3, 2021

/gha run r-solaris

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

solaris-x86-patched: https://builder.r-hub.io/status/lightgbm_3.2.1.99.tar.gz-3701ffb1819540bf98d77218c211cd5c
solaris-x86-patched-ods: https://builder.r-hub.io/status/lightgbm_3.2.1.99.tar.gz-481aa16dd0084b4c9bae6fc8a0043cd7
Reports also have been sent to LightGBM public e-mail: http://www.yopmail.com/lightgbm_rhub_checks
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.

Thank you very much for the iterative improvement!

@jameslamb jameslamb merged commit bb88d92 into microsoft:master May 3, 2021
@jameslamb jameslamb deleted the r/sexp-pointers branch May 3, 2021 18:59
@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.

(R) Functions that replace R headers are not compatible with all R versions
2 participants