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] [docs] fix typo in documentation on init_model #5379

Merged
merged 3 commits into from
Jul 28, 2022

Conversation

sebffischer
Copy link
Contributor

@sebffischer sebffischer commented Jul 18, 2022

Fixes #5377.

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 for this!

First, can you please re-generate the documentation for the package? We keep the .Rd files (these are what R uses to power the help docs shown with e.g. ?lightgbm::lightgbm) in source control.

You could do that by running the following:

sh build-cran-package.sh --no-build-vignettes
R CMD INSTALL --with-keep.source ./lightgbm_3.3.2.99.tar.gz
cd R-package
Rscript --vanilla -e "roxygen2::roxygenize(load = 'installed')"

If you don't want to do that, please give me access to your fork and I'd be happy to make those changes and push them to this PR.


Second, I've added fixes #5377 to the description. Whenever you open a pull request here, please provide a description that helps maintainers to understand why you're proposing what you're proposing. And whenever some prior discussions on GitHub led to the PR or are relevant to it, please link them.


In the future when you contribute here or in other projects on GitHub, I encourage you to create new branches on your fork instead of modifying master / main. That way:

  • you can work on multiple pull requests at the same time
  • it's easier to ensure your fork's master / main branch will have the same history as the one in the upstream project when changes are made upstream

Let me know if you have any questions, I'm here to help 👋

@jameslamb jameslamb added the doc label Jul 18, 2022
@jameslamb jameslamb changed the title docs: typo in documentation of parameter init_model [R-package] [docs] fix typo in documentation on init_model Jul 18, 2022
R-package/R/lightgbm.R Outdated Show resolved Hide resolved
@sebffischer
Copy link
Contributor Author

Thanks! Maybe the instructions on how to build the docs could be added here

I ran devtools::document() which did not work and then I - incorrectly - assumed the docs might be build automatically in the CI.

Also one has to run git submodule update --init --recursive before to get the external libs

@ghost
Copy link

ghost commented Jul 19, 2022

CLA assistant check
All CLA requirements met.

@jameslamb
Copy link
Collaborator

Maybe the instructions on how to build the docs could be added

Yep you're right! Sorry about that. I promised @jmoralez I'd do that a long time ago and never did 🙈 . Will do that right now.

one has to run git submodule update --init --recursive

Yes that's right. That is mentioned in multiple places in https://github.com/microsoft/LightGBM/tree/master/R-package and https://lightgbm.readthedocs.io/en/v3.3.2/Installation-Guide.html, although in a slightly different way.

LightGBM's documentation recommends setting up all the submodules at the time that you clone, like this:

git clone \
    --recursive \
    https://github.com/microsoft/LightGBM

But if you've already cloned without --recursive, then you're correct that updating them for your existing repo requires running

git submodule init
git submodule update --recursive

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.

Changes look great to me, thanks very much for your contribution and for using LightGBM 🎉

@jameslamb
Copy link
Collaborator

Sorry for the delay! We had some continuous integration (CI) problems to fix, which have been resolved in #5388 .

I just updated this PR to the latest changes in master, and once CI runs successfully I'll merge it.

@jameslamb jameslamb merged commit 68b7315 into microsoft:master Jul 28, 2022
@jameslamb
Copy link
Collaborator

Thanks again for the contribution @sebffischer !

Now that this is merged, master here in LightGBM will not match master on your fork. I recommend deleting your fork of the repo. If you come back to contribute in the future (we hope you will!), use a branch other than master and you won't have to worry about it.

@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 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.

[R-package] init_model should accept booster
4 participants