-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
[docs] Clarify the fact that predict() on a file does not support saved Datasets (fixes #4034) #4545
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.
Thank you for picking this up! I have some comments below:
Co-authored-by: Nikita Titov <[email protected]>
… into fix/predict-from-file
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.
Thank you! LGTM, except one minor unifying suggestion.
R-package/R/lgb.Dataset.R
Outdated
#' a character representing a path to a text file (CSV, TSV, or LibSVM) | ||
#' or a LightGBM Dataset binary file |
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.
Let's make identical descriptions really identical without paraphrasing. Users might spend their time trying to find a difference where there is actually no any difference. Also, it will help us to not miss all occurrences of identical parameters during possible future updates.
#' a character representing a path to a text file (CSV, TSV, or LibSVM) | |
#' or a LightGBM Dataset binary file | |
#' a character representing a path to a text file (CSV, TSV, or LibSVM), | |
#' or a character representing a path to a binary \code{Dataset} file |
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.
oh sure, seems fine to me!
I'll do this one manually instead of applying in the browser, since the corresponding lgb.Dataset.Rd
will also have to be regenerated.
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.
updated in de189a6, thanks as always for being thorough! You're right, that will make it easier to catch all such phrases in the future.
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. |
Fixes #4034.
Contributes to #4310.
predict()
methods in LightGBM can be used with data stored in a text file (CSV, TSV, or LibSVM). Those methods do not support creating predictions on other formats like LightGBMDataset
objects saved in a binary file.This PR proposes the following changes to fix #4034:
Booster$eval()
on a constructedDataset
stored in a file ([R-package] predict() breaks when using a Dataset stored in a file #4034 (comment))