-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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] introduce Dataset methods set_field() and get_field() #4571
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure whether we should update here
https://github.com/microsoft/LightGBM/blob/master/docs/FAQ.rst#i-used-setinfo-tried-to-print-my-lgb-dataset-and-now-the-r-console-froze. WDYT?
Update Dataset$setinfo()
here
LightGBM/R-package/R/lgb.Dataset.R
Line 557 in 54facc4
, "To modify attributes like 'init_score', use Dataset$setinfo(). " |
Co-authored-by: Nikita Titov <[email protected]>
Done in a745987
I think it's ok to leave that alone for now, since the functions are being deprecated but not removed yet. That entire FAQ section probably deserves some updates and revisions, to check if the issues listed there still exist in recent versions. But that could be done in a separate PR. |
iiinteresting, I see this error on the
I did re-generate the docs before pushing. Maybe there was a new release of |
Yes, there was a new release (7.1.2) a few days ago: https://cran.r-project.org/web/packages/roxygen2/index.html. But it also looks like there are a few local changes I forgot to Regenerated the docs with |
Other modifications LGTM. They seem to be simple renaming. And thanks @StrikerRUS for detecting the minor mistakes. |
ah! I answered this in #4571 (comment) actually. I don't think we need to worry about it for this PR. That particular question was added in #976 (4 years ago), so it might not even be relevant any more. I'd prefer to take updating the FAQ as a separate task. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks a lot for working on this!
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. |
Contributes to #4543, as part of #4310.
This PR proposes the following changes in the R package:
Dataset$getinfo()
(with a warning its name will be changed toget_field()
)Dataset$setinfo()
(with a warning its name will be changed toset_field()
)Dataset$get_field(field_name)
toDataset
, matching the Python packageLightGBM/python-package/lightgbm/basic.py
Line 1939 in 8a90ea3
Dataset$set_field(field_name, data)
should be added toDataset
, matching the Python packageLightGBM/python-package/lightgbm/basic.py
Line 1890 in 8a90ea3
Notes for Reviewers
I only added new tests, but left existing tests with
getinfo()
andsetinfo()
in place, so that those methods are still tested as long as they exist in the package.