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

[python-package] Allow to pass Arrow array as weights #6164

Merged
merged 56 commits into from Nov 13, 2023
Merged

[python-package] Allow to pass Arrow array as weights #6164

merged 56 commits into from Nov 13, 2023

Conversation

borchero
Copy link
Collaborator

Motivation

This is part of a series of PRs towards #6022 and depends on #6163.

For a legible diff, see borchero/LightGBM@arrow-support-labels...arrow-support-weights.

@borchero borchero changed the title WIP: [python-package] Allow to pass Arrow array as weights [python-package] Allow to pass Arrow array as weights Nov 7, 2023
@borchero borchero marked this pull request as ready for review November 7, 2023 22:57
@borchero
Copy link
Collaborator Author

borchero commented Nov 7, 2023

@jameslamb I think you might need to request reviewers manually since they were removed earlier 👀

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.

Excellent work! Thanks so much for the thorough tests, for bringing in pyarrow.compute through the same way this project handles other conditional imports, and for switching over the tests to using that np_assert_array_equal() function.

I left some minor comments, but overall I'm very happy with this addition to the Python API. Like in the other PRs, I'd like @guolinke or @shiyu1994 to review as well, particularly the C/C++ changes.

Don't worry about this lint CI job complaining about the R package: https://github.com/microsoft/LightGBM/actions/runs/6791304427/job/18462554522?pr=6164

That's failing for reasons unrelated to this PR and I'll put up a PR soon to fix it.

python-package/lightgbm/basic.py Show resolved Hide resolved
tests/python_package_test/test_arrow.py Outdated Show resolved Hide resolved
tests/python_package_test/test_arrow.py Outdated Show resolved Hide resolved
tests/python_package_test/test_arrow.py Show resolved Hide resolved
@jameslamb jameslamb self-requested a review November 8, 2023 14:43
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.

Looks great to me, thanks for the changes!

Let's wait for one more approval on the C/C++ changes.

Copy link
Collaborator

@guolinke guolinke left a comment

Choose a reason for hiding this comment

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

LGTM

@jameslamb
Copy link
Collaborator

Thanks @borchero and to @guolinke for the review.

Please move on to the next PR.

@jameslamb jameslamb merged commit deb7077 into microsoft:master Nov 13, 2023
41 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants