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] Add remainder of prediction funtions #5312

Merged
merged 39 commits into from
Aug 23, 2022

Conversation

david-cortes
Copy link
Contributor

This PR adds the remainder of prediction functions from PR #4982 which were not included in PR #5108 . The idea in #4982 was to split the additions into multiple PRs, but it's getting very complicated to merge branches and to remember what were the changes after the ellapsed amount of time and PRs in between, so this PR just adds the rest of C prediction functions that are not in the R interface together.

It should be pointed out again from issue #4980 that there are some differences between the documentation of some existing C functions added to the R interface here and their actual behavior, and as such the docs will need to be updated once those issues are sorted out.

Copy link
Collaborator

@jameslamb jameslamb 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, thanks for doing this! We can proceed with this large PR. I'll do my best to review it thoroughly as quickly as possible.

Please see my first round of suggestions, based on a quick read of the PR's contents.

R-package/R/lgb.Predictor.R Outdated Show resolved Hide resolved
R-package/R/lgb.Booster.R Outdated Show resolved Hide resolved
R-package/R/lgb.Booster.R Outdated Show resolved Hide resolved
R-package/src/lightgbm_R.cpp Outdated Show resolved Hide resolved
R-package/R/lgb.Booster.R Show resolved Hide resolved
@david-cortes
Copy link
Contributor Author

Updated.

Copy link
Collaborator

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

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

Finally, a feature making it into the R package before the Python package 🎉 . Thanks so much for your help with this!

I'm ready to start actively reviewing this PR and working with you on getting it merged. Overall, I support the approach taken here. I support the introduction of lgb.configure_fast_predict() and really appreciate the detailed documentation and tests.

Please see my next round of review comments. Please also update this to the latest state of the master branch.

I'd like to also run the project's valgrind checks over this code, although right now that CI job is failing on master (#5403 (comment)).

@shiyu1994 or @guolinke once this PR has been updated to latest master, can you please review these changes? Especially in lightgbm_R.cpp and lightgbm_R.h.

R-package/R/lgb.Predictor.R Outdated Show resolved Hide resolved
R-package/R/lgb.Predictor.R Outdated Show resolved Hide resolved
R-package/R/lgb.Predictor.R Outdated Show resolved Hide resolved
R-package/R/lgb.Predictor.R Outdated Show resolved Hide resolved
R-package/R/lgb.Predictor.R Outdated Show resolved Hide resolved
R-package/R/lgb.Booster.R Outdated Show resolved Hide resolved
R-package/R/lgb.Booster.R Outdated Show resolved Hide resolved
R-package/R/lgb.Booster.R Show resolved Hide resolved
R-package/R/lgb.Predictor.R Outdated Show resolved Hide resolved
R-package/R/utils.R Show resolved Hide resolved
david-cortes and others added 11 commits August 16, 2022 15:54
Co-authored-by: James Lamb <jaylamb20@gmail.com>
Co-authored-by: James Lamb <jaylamb20@gmail.com>
Co-authored-by: James Lamb <jaylamb20@gmail.com>
Co-authored-by: James Lamb <jaylamb20@gmail.com>
Co-authored-by: James Lamb <jaylamb20@gmail.com>
Co-authored-by: James Lamb <jaylamb20@gmail.com>
Co-authored-by: James Lamb <jaylamb20@gmail.com>
Co-authored-by: James Lamb <jaylamb20@gmail.com>
Co-authored-by: James Lamb <jaylamb20@gmail.com>
@david-cortes
Copy link
Contributor Author

@shiyu1994
Copy link
Collaborator

@guolinke Could you please kindly review this, if you have time? Thanks.

Copy link
Collaborator

@guolinke guolinke left a comment

Choose a reason for hiding this comment

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

The cpp part looks just called c_apis, so it looks good to me.

Copy link
Collaborator

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

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

Thanks so much for this excellent work! I noticed one typo in docs, but otherwise I am good with these changes and excited to merge them into the project. 🎉

Leaving a "request changes" review just to block merging until we have a chance to run the project's valgrind checks over this PR. And that can't be done until we fix those tests (ongoing work in #5429).

Until then... @jmoralez would you please review this when you have time?

R-package/src/lightgbm_R.h Outdated Show resolved Hide resolved
R-package/src/lightgbm_R.h Outdated Show resolved Hide resolved
R-package/src/lightgbm_R.h Outdated Show resolved Hide resolved
R-package/src/lightgbm_R.h Outdated Show resolved Hide resolved
david-cortes and others added 4 commits August 19, 2022 22:57
Co-authored-by: James Lamb <jaylamb20@gmail.com>
Co-authored-by: James Lamb <jaylamb20@gmail.com>
Co-authored-by: James Lamb <jaylamb20@gmail.com>
Co-authored-by: James Lamb <jaylamb20@gmail.com>
@jameslamb
Copy link
Collaborator

jameslamb commented Aug 19, 2022

/gha run r-valgrind

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

Status: success ✔️.

Copy link
Collaborator

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

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

Looks good to me! I'll leave this open for a few days in case @jmoralez wants to review.

@jmoralez
Copy link
Collaborator

Sorry I haven't had much time. I'll review this tomorrow

@jameslamb
Copy link
Collaborator

Thanks @jmoralez !

Copy link
Collaborator

@jmoralez jmoralez left a comment

Choose a reason for hiding this comment

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

Awesome contribution!

@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 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants