Skip to content

[cpp] rename is_finished to produced_empty_tree in cpp and R code #6970

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

StrikerRUS
Copy link
Collaborator

@StrikerRUS StrikerRUS commented Jul 14, 2025

Refer to #6890 (comment).

I did not want to touch R and C code and docs. Consequently the name and doc of the return value is inconsistent between the python, C, and R code and docs.

@@ -759,7 +759,11 @@ LIGHTGBM_C_EXPORT int LGBM_BoosterGetNumClasses(BoosterHandle handle,
/*!
* \brief Update the model for one iteration.
* \param handle Handle of booster
* \param[out] is_finished 1 means the update was successfully finished (cannot split any more), 0 indicates failure
* \param[out] is_finished 1 means the tree(s) produced by this iteration did not have any splits.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think that it worth introducing a breaking change for renaming this param. I believe that changing the description is enough.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Apologies if I am showing my ignorance here, but I don't think that renaming the argument to a C function is an API-breaking or ABI-breaking change, because C functions only accept positional arguments.

I don't think renaming is_finished to produced_empty_tree here would break any existing code.

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 a lot for your early feedback!
You're totally right! Pushed changes in f16671c.

Now the only left place with is_finished is

bool is_finished = false;
auto start_time = std::chrono::steady_clock::now();
for (int iter = 0; iter < config_->num_iterations && !is_finished; ++iter) {
is_finished = TrainOneIter(nullptr, nullptr);
if (!is_finished) {
is_finished = EvalAndCheckEarlyStopping();
}

I think it's correct naming because this variable is used as a return value not only from TrainOneIter() but also from EvalAndCheckEarlyStopping().

@StrikerRUS StrikerRUS marked this pull request as ready for review July 15, 2025 15:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants