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] remove unused callback cb.reset.parameter #4871

Merged
merged 1 commit into from
Dec 11, 2021

Conversation

jameslamb
Copy link
Collaborator

Proposes removing the callback function cb.reset.parameter() in the R package's code.

Notes for Reviewers

All callback functions in the R package are currently unexported (see #2479). Any that are unused can be safely removed without affecting users.

cb.reset.parameter() is unused in the package. It was added in the very first PR introducing the R package (#168), I think probably just because the same callback existed in the Python package.

This PR proposes removing it to simplify the R package's code.

@StrikerRUS
Copy link
Collaborator

Why remove but not expose (as per #2479)?

@jameslamb
Copy link
Collaborator Author

Why remove but not expose (as per #2479)?

The code for cb.reset.parameter() was added to this project around 5 years ago and hasn't been tested or used at any point during that time. So I don't know if it actually works with the current state of LightGBM, or even if it ever worked.

I think that completing #2479 will require a medium-sized effort...it will involve adding documentation and tests, for example. I'm personally not ready to put in that effort yet, as I'm prioritizing some other work on this project higher.

So I thought "as long as the callbacks are not actually accessible to users, might as well remove this one that has never been used, to simplify the package's code".

@StrikerRUS
Copy link
Collaborator

StrikerRUS commented Dec 10, 2021

So I don't know if it actually works with the current state of LightGBM, or even if it ever worked.

Ah, OK. My point was to preserve callbacks code with the aim to not reimplement them again when expose to public API. But thanks to GitHub cross-references, one still can copy-paste removed code from this PR if needed.

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.

LGTM!

@jameslamb jameslamb merged commit 0e25841 into master Dec 11, 2021
@jameslamb jameslamb deleted the fix/remove-reset-parameter-callback branch December 11, 2021 02:22
@StrikerRUS StrikerRUS mentioned this pull request Jan 6, 2022
13 tasks
@jameslamb jameslamb mentioned this pull request Oct 7, 2022
40 tasks
@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