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] update lgb.Dataset.R to use keyword arguments #3607

Merged
merged 6 commits into from
Nov 29, 2020
Merged

[R package] update lgb.Dataset.R to use keyword arguments #3607

merged 6 commits into from
Nov 29, 2020

Conversation

zenggyu
Copy link
Contributor

@zenggyu zenggyu commented Nov 29, 2020

Addresses part of #3390. Two notes:

  1. There are a few functions that have only one argument(e.g., lgb.is.Dataset(x)). I don't know if it is necessary to modify these function calls to use keyword arguments; if it is, please let me know and I'll make a new commit to modify all these function calls.
  2. I am not so sure about line 280. Is it correct that the first argument of the call private$predictor$predict() is data?

Thank you @jameslamb for providing this learning opportunity! I am already learning something new about git and github!

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 again for taking time to contribute to LightGBM!

I left some small suggestions. Please also fix the linting errors listed in https://travis-ci.org/github/microsoft/LightGBM/jobs/746519935. You can reproduce these errors locally by running Rscript .ci/lint_r_code.R $(pwd).

@zenggyu one other question...are you interested in completely finishing #3390 ? I originally set it up file-by-file like that for Hacktoberfest, but now that Hacktoberfest is over I'd like to finish it. If you have the time and interest, I'd be grateful if you'd handle the rest of the files there in one or two additional pull requests after this one (doesn't have to be one-fiile-per-PR).

R-package/R/lgb.Dataset.R Outdated Show resolved Hide resolved
@@ -277,7 +277,7 @@ Dataset <- R6::R6Class(

# Setup initial scores
init_score <- private$predictor$predict(
private$raw_data
data = private$raw_data
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is correct! Just pointing out since you weren't sure, this is an instance of Predictor and comes from

predict = function(data,

@jameslamb
Copy link
Collaborator

@zenggyu it looks like many of your commits were done under a profile that is not linked to your GitHub Account. I can see this because your profile picture doesn't show up next to them.

image

I recommend that when you work on open source stuff, you set your gitconfig with an email address tied to your GitHub Account. This will make sure your user has credit in projects' "Contributors" list, and that the GitHub activity overview is accurate.

I described how to fix this in uptake/pkgnet#284 (review). If you try that and get stuck, let me know and I'd be happy to help.

@zenggyu
Copy link
Contributor Author

zenggyu commented Nov 29, 2020

Thanks for telling me! I changed my email setting in git config a few days ago and I would prefer to use this new address. Let me try changing the primary email address of my github account (this should not require any changes in the git config right?).

@zenggyu
Copy link
Contributor Author

zenggyu commented Nov 29, 2020

图片

OK, I think I have fixed it.

@jameslamb
Copy link
Collaborator

OK, I think I have fixed it.

yep, looks good!

R-package/R/lgb.Dataset.R Outdated Show resolved Hide resolved
Co-authored-by: James Lamb <jaylamb20@gmail.com>
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!

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

3 participants