-
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
[R-package] fix R examples and lgb.plot.interpretation() #3002
Conversation
Building this branch right now. |
It is failing due to wrong R version of
|
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.
lgb.Dataset.set.categorical
- FIXED: https://lightgbm.readthedocs.io/en/jameslamb-fix-r-examples/R/reference/lgb.Dataset.set.categorical.htmllgb.plot.interpretation
- FIXED: https://lightgbm.readthedocs.io/en/jameslamb-fix-r-examples/R/reference/lgb.plot.interpretation.htmllgb.cv
- FIXED: https://lightgbm.readthedocs.io/en/jameslamb-fix-r-examples/R/reference/lgb.cv.html
I believe we should bump version to LightGBM/R-package/DESCRIPTION Line 36 in 995a597
and correct the FAQ entry, because not only |
I disagree about updating the version floor. It seems that this is not specific to I do agree on updating the FAQ entry...I'll do that directly in this PR. |
@StrikerRUS are you ok with the update I made to the FAQ? I think it came after your last approval. |
@jameslamb I'm OK with that you are against bumping the required version. But the FAQ entry should state that in case of that error version upgrade is required anyway for someone who encoutrered it, I believe. Sure, please do it right in this PR. |
ha I forgot to push the commit!! I pushed to Here it is: 0eb0243 |
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.
Ah, I see! FAQ update looks good. Thanks a lot for fixing all issues!
Since this one is a user-facing fix (fixing Thanks for the help @StrikerRUS ! |
*NOTE: Moving #2999 over here (based on a LightGBM branch) so we can test readthedocs builds. See #2999 (comment)
See comments from @StrikerRUS
A few of the R examples are broken right now. These weren't caught by tests because one is only a warning, one is a plot that is generated successfully but is incorrect, and one is an issue specific to our readthedocs builds.
I think the issue for
lgb.cv()
is #2715. In that issue we found thatdata.table 1.11.4
and R 3.6.0 lead to a data.table error ending incolumn or argument 2 is NULL
. I think this can be fixed by upgrading to thedata.table
1.12.1 inconf.py
.To fix the
lgb.set.categorical()
example, I changed file names across several examples to be sure that no two examples write to the same file name.This issue with
lgb.plot.interpretation()
is because I made a change and forgot to useabs()
.new plot:
I was not able to reproduce the
data.table
error in #2989 (comment). It