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] Convert LGBM_GetLastError_R to use R built-in types #4242

Merged
merged 8 commits into from
May 2, 2021

Conversation

jameslamb
Copy link
Collaborator

@jameslamb jameslamb commented Apr 30, 2021

This is a small step towards #3016. This PR proposes converting LGBM_GetLastError_R (and the R function that calls it, lgb.last_error()) to use R's built-in types instead of the custom type defined in https://github.com/microsoft/LightGBM/blob/91f72e2a8c8b1e5433d30d3891923b3a067402a6/R-package/src/R_object_helper.h.

Notes for Reviewers

This PR is not dependent on #4155. #4155 has been merged to master and this PR has been updated with it.

You can see the following to learn about the SEXPtype.

@jameslamb jameslamb changed the title WIP: [R-package] Convert LGBM_GetLastError_R to use R built-in types [R-package] Convert LGBM_GetLastError_R to use R built-in types Apr 30, 2021
@jameslamb jameslamb marked this pull request as ready for review April 30, 2021 13:50
@jameslamb jameslamb requested a review from Laurae2 as a code owner April 30, 2021 13:50
@jameslamb
Copy link
Collaborator Author

Ok CI seems to be happy, so I've converted this to "ready for review".

@jameslamb
Copy link
Collaborator Author

jameslamb commented May 1, 2021

/gha run r-valgrind

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

Status: failure ❌.

@jameslamb
Copy link
Collaborator Author

jameslamb commented May 1, 2021

/gha run r-valgrind

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

Status: success ✔️.

@jameslamb
Copy link
Collaborator Author

jameslamb commented May 1, 2021

/gha run r-solaris

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

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

Very nice simplification!

Comment on lines +61 to +62
out = PROTECT(Rf_allocVector(STRSXP, 1));
SET_STRING_ELT(out, 0, Rf_mkChar(LGBM_GetLastError()));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just a question: what is the difference between Rf_allocVector/allocVector and Rf_mkChar/mkChar?
I could find only the following link: https://www.cs.kent.ac.uk/projects/cxxr/doc/html/Rinternals_8h.html. And it looks like they are just aliases...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yep, just aliases! This #define says "use the symbols with the Rf_ prefix"

#define R_NO_REMAP

I like that because it makes it a little more obvious which symbols are coming from R's headers.

@jameslamb jameslamb merged commit 66ee291 into microsoft:master May 2, 2021
@jameslamb jameslamb deleted the fix/sexp branch May 2, 2021 21:23
@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