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

Add C API function that returns all parameter names with their aliases #4829

Merged
merged 17 commits into from
Dec 3, 2021

Conversation

StrikerRUS
Copy link
Collaborator

Fixed #2633

@StrikerRUS StrikerRUS changed the title Add C API function that returns all parameter names with their aliases [WIP] Add C API function that returns all parameter names with their aliases Nov 27, 2021
R-package/R/aliases.R Outdated Show resolved Hide resolved
@StrikerRUS StrikerRUS changed the title [WIP] Add C API function that returns all parameter names with their aliases Add C API function that returns all parameter names with their aliases Nov 27, 2021
@StrikerRUS
Copy link
Collaborator Author

OK, I believe this PR is ready for reviews.

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.

This is awesome! I totally support this addition to the API. I think it'll help to avoid some maintenance burden in language wrappers and will eliminate some possible bugs. Great work 🤩

Please see a few small comments I left.

R-package/R/aliases.R Outdated Show resolved Hide resolved
R-package/R/aliases.R Outdated Show resolved Hide resolved
include/LightGBM/config.h Outdated Show resolved Hide resolved
tests/python_package_test/test_basic.py Show resolved Hide resolved
python-package/lightgbm/basic.py Outdated Show resolved Hide resolved
Co-authored-by: James Lamb <jaylamb20@gmail.com>
@jameslamb jameslamb self-requested a review December 2, 2021 02:30
@StrikerRUS
Copy link
Collaborator Author

@jameslamb Thanks a lot for your review comments! I think I've addressed them all in the latest commit. Please take another look when have time.

@jameslamb
Copy link
Collaborator

jameslamb commented Dec 2, 2021

/gha run r-valgrind

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

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 great, thanks for the changes, especially the new R test!

Since this touched lightgbm_R.cpp, I just did a valgrind run (sorry for not thinking of that sooner). #4829 (comment)

As soon as that passes, I'm comfortable with merging this.

@jameslamb
Copy link
Collaborator

@shiyu1994 do you support this addition to c_api.cpp? I think changes to that file should probably have two approvals, just since it is the core public API of LightGBM.

Copy link
Collaborator

@shiyu1994 shiyu1994 left a comment

Choose a reason for hiding this comment

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

@StrikerRUS @jameslamb Thank you for working on this! I've checked the C API, and the changes LGTM.

@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.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add C API function that returns all parameter names with their aliases
3 participants