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] Turn matrix to storage mode "double" #3140

Merged
merged 5 commits into from
Jun 4, 2020
Merged

[R-package] Turn matrix to storage mode "double" #3140

merged 5 commits into from
Jun 4, 2020

Conversation

mayer79
Copy link
Contributor

@mayer79 mayer79 commented Jun 2, 2020

Fixes #3139

Just copy-pasted code snippet from lgb.Dataset initalizer.

As I am still not very familiar with the LightGBM data logic, @Laurae2 and @jameslamb would need to confirm that this makes any sense.

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 for the investigation and PR @mayer79 ! Could you please add a new file R-package/tests/test_Predictor.R and add a unit test on this behavior? Whenever we fix a bug, we should also include tests to be sure we don't re-introduce it in the future.

@Laurae2
Copy link
Contributor

Laurae2 commented Jun 2, 2020

@jameslamb This issue will also solve #3094

@ghost
Copy link

ghost commented Jun 2, 2020

CLA assistant check
All CLA requirements met.

@StrikerRUS StrikerRUS changed the title Turn matrix to storage mode "double" [R-package] Turn matrix to storage mode "double" Jun 2, 2020
@jameslamb jameslamb self-requested a review June 2, 2020 21:28
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.

The test looks good! I left a few additional small requests

R-package/R/lgb.Predictor.R Outdated Show resolved Hide resolved
R-package/tests/testthat/test_Predictor.R Outdated Show resolved Hide resolved
mayer79 and others added 2 commits June 3, 2020 07:52
@jameslamb jameslamb self-requested a review June 4, 2020 02:47
@jameslamb
Copy link
Collaborator

thanks for the contribution and for using LightGBM, @mayer79 !

@jameslamb jameslamb merged commit 9d43303 into microsoft:master Jun 4, 2020
ChipKerchner pushed a commit to ChipKerchner/LightGBM that referenced this pull request Jun 10, 2020
* Turn matrix to storage mode "double"

* added "test_Predictor.R" to R tests with check for integer storage mode during prediction

* Explicit integers in test_Preditor to avoid TravisCI failure

* properly aligning comment on storage.mode

Co-authored-by: James Lamb <jaylamb20@gmail.com>

* split double assignment into two lines

Co-authored-by: James Lamb <jaylamb20@gmail.com>
ChipKerchner pushed a commit to ChipKerchner/LightGBM that referenced this pull request Jun 11, 2020
* Turn matrix to storage mode "double"

* added "test_Predictor.R" to R tests with check for integer storage mode during prediction

* Explicit integers in test_Preditor to avoid TravisCI failure

* properly aligning comment on storage.mode

Co-authored-by: James Lamb <jaylamb20@gmail.com>

* split double assignment into two lines

Co-authored-by: James Lamb <jaylamb20@gmail.com>
@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.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

prediction failure with integer data
4 participants