[R-package] clarify parameter documentation (fixes #4193) #4202
Conversation
| #' @param callbacks List of callback functions that are applied at each iteration. | ||
| #' @param reset_data Boolean, setting it to TRUE (not the default value) will transform the | ||
| #' booster model into a predictor model which frees up memory and the | ||
| #' original datasets | ||
| #' @param ... other parameters, see Parameters.rst for more information. A few key parameters: | ||
| #' @param ... other parameters, see \href{https://lightgbm.readthedocs.io/en/latest/Parameters.html}{ |
There was a problem hiding this comment.
Just a question: how this differs from params (the first one) argument?
There was a problem hiding this comment.
unfortunately, they both can take the exact same information. And then they get merged together:
LightGBM/R-package/R/lgb.train.R
Lines 85 to 86 in 8e126c8
That has been a part of this project's R package since before I joined LightGBM, so for several years. Now that we're saying that the next release will be 4.0.0 and breaking changes will be allowed, I would love to get rid of support for ... and force people to be more explicit. I think it introduces extra complexity for no benefit.
There was a problem hiding this comment.
There is a very real functional downside of allowing ..., I should have mentioned it in my comment. If you misspell one of the keyword arguments, it will just be assumed that you are referring to a LightGBM parameter.
So if you use, say, lgb.train(resetting_data = TRUE), training will proceed with reset_data still set to the default (TRUE) and you'll get a warning in logs like [LightGBM ] [WARN] unrecognized parameter: 'resetting_data'.
If we didn't support ..., then you'd get an error (not a warning) in R immediately like "unrecognized argument: resetting_data", which I think is desirable.
There was a problem hiding this comment.
I would love to get rid of support for
...
Yeah, good idea I think!
There was a problem hiding this comment.
I'll add a feature request. And @ some of the people who've opened R issues here in the past for their thoughts.
There was a problem hiding this comment.
I just created #4226
|
I did check https://lightgbm.readthedocs.io/en/docs-jlamb/R/reference/ to be sure this is looking ok. The RTD build passed (https://readthedocs.org/projects/lightgbm/builds/) and visually it seems good to me so I'll merge. for example, https://lightgbm.readthedocs.io/en/docs-jlamb/R/reference/lgb.train.html
|
|
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. |
This PR proposes some clarifications in the R documentation, based on the feedback in #4193.
Notes for Reviewers
Pushing to the branch
docs/jlambso we can check the rendered docs on readthedocs.