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] expose start_iteration to dump/save/lgb.model.dt.tree #6398

Merged
merged 30 commits into from May 16, 2024

Conversation

mayer79
Copy link
Contributor

@mayer79 mayer79 commented Apr 1, 2024

Fixes #6397

Contributes to #6380

@mayer79 mayer79 marked this pull request as draft April 1, 2024 12:21
@mayer79 mayer79 marked this pull request as ready for review April 21, 2024 06:51
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 for this! Now that CI seems to be working again, let's continue with this. I left some initial suggestions and questions for your consideration, but overall I'm very supportive of this addition to the R package's API.

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 Outdated Show resolved Hide resolved
R-package/R/lgb.model.dt.tree.R Outdated Show resolved Hide resolved
R-package/tests/testthat/test_lgb.Booster.R Outdated Show resolved Hide resolved
R-package/tests/testthat/test_lgb.Booster.R Outdated Show resolved Hide resolved
R-package/tests/testthat/test_lgb.Booster.R Show resolved Hide resolved
R-package/tests/testthat/test_lgb.Booster.R Outdated Show resolved Hide resolved
R-package/tests/testthat/test_lgb.Booster.R Show resolved Hide resolved
@mayer79
Copy link
Contributor Author

mayer79 commented Apr 23, 2024

@jameslamb Increadible review, thank you so much for your dedication!

Some comments:

  1. Now, the user sees 1-based start_iteration. The conversion to 0-based is done before calling the R wrappers of the C_API. We could also move the 0-basing into the R wrappers.
  2. The new argument start_iteration has moved to the end of all argument lists, also those of the R wrappers of the C++ code.
  3. I have left the weak unit test of $save_model_to_string() because I don't know how to parse the content of the string.
  4. Also the unit test of lgb.save() is untouched because serializing and deserializing loses the tree index. At least I have this in mind from when I tried to improve the test. Maybe you have a good idea how to make this better?

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!

Thanks for working with me on the unit tests. I'm really happy that this PR is going to add so much better coverage of an under-tested part of the library.

I left one more set of small suggestions, and then I think this is ready to merge 🎉

R-package/tests/testthat/test_lgb.Booster.R Outdated Show resolved Hide resolved
R-package/tests/testthat/test_lgb.Booster.R Outdated Show resolved Hide resolved
R-package/tests/testthat/test_lgb.Booster.R Outdated Show resolved Hide resolved
R-package/tests/testthat/test_lgb.Booster.R Outdated Show resolved Hide resolved
R-package/tests/testthat/test_lgb.Booster.R Outdated Show resolved Hide resolved
R-package/R/lgb.model.dt.tree.R Outdated Show resolved Hide resolved
@mayer79
Copy link
Contributor Author

mayer79 commented May 12, 2024

Thanks so much!

Thanks for working with me on the unit tests. I'm really happy that this PR is going to add so much better coverage of an under-tested part of the library.

I left one more set of small suggestions, and then I think this is ready to merge 🎉

Perfect! Should we reformulate the docstrings of all affected functions?

@jameslamb
Copy link
Collaborator

yes please

@mayer79
Copy link
Contributor Author

mayer79 commented May 12, 2024

yes please

done.

@jameslamb jameslamb self-requested a review May 16, 2024 03:32
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 very much!

@jameslamb jameslamb merged commit e0ac635 into microsoft:master May 16, 2024
38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[R-package] Expose start_iteration to dump/save/lgb.model.dt.tree
2 participants