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] Sort indices in lgb.cv() (fixes #2503) #2524

Merged
merged 1 commit into from
Oct 24, 2019

Conversation

jameslamb
Copy link
Collaborator

This PR attempts to fix the issue reported by @rgranvil in #2503 . You can confirm by building the R package from this branch and running the sample code provided in that issue.

I effectively just mirrored what was done on the Python side in #2510 .

@StrikerRUS
Copy link
Collaborator

Sorry, don't have R environment to confirm the fix.

Copy link
Contributor

@Laurae2 Laurae2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to work correctly here.

@jameslamb jameslamb merged commit bdc310a into microsoft:master Oct 24, 2019
@guolinke
Copy link
Collaborator

@jameslamb did you check these ?

dtrain <- slice(data, seq_len(nrow(data))[-folds[[k]]])
setinfo(dtrain, "weight", getinfo(data, "weight")[-folds[[k]]])
setinfo(dtrain, "init_score", getinfo(data, "init_score")[-folds[[k]]])
setinfo(dtest, "weight", getinfo(data, "weight")[folds[[k]]])
setinfo(dtest, "init_score", getinfo(data, "init_score")[folds[[k]]])

for the indices in weight and init_score.

@guolinke
Copy link
Collaborator

ping @jameslamb @Laurae2 again

@jameslamb
Copy link
Collaborator Author

@guolinke I did not. Looking...

@guolinke
Copy link
Collaborator

guolinke commented Nov 4, 2019

@jameslamb
I am afraid that the index orders used in feature and (weight, label, ...) are different.

@StrikerRUS
Copy link
Collaborator

@jameslamb Any updates?

@jameslamb
Copy link
Collaborator Author

@StrikerRUS @guolinke sorry for the delay! I've opened #2572 to track this, so we can have the discussion in an open issue instead of in a comment on a closed PR.

I have a PR to fix this coming momentarily.

@StrikerRUS
Copy link
Collaborator

@jameslamb Thanks! I was worrying that this issue can be lost in the closed PR.

@jameslamb jameslamb deleted the bugfix/index_sort branch January 27, 2020 00:15
@StrikerRUS StrikerRUS added the fix label Mar 1, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Mar 27, 2020
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.

4 participants