-
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
[dask] [python-package] use keyword args for internal function calls #3755
Conversation
I have some improvements in my mind that I haven't written in my review too. But all massive rewrites of dask.py module will introduce a lot of merge conflicts with #3708. WDYT, is it OK or we should merge #3708 first? |
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.
LGTM except 1 typo!
But pleeeease do not adopt old-fashioned 80-char line length limit 😃 !
#2437 (comment)
Co-authored-by: Nikita Titov <nekit94-08@mail.ru>
Thanks!
I looked at the diff in #3708 and I don't think the changes in this PR are going to cause too many issues. Some of these changes are actually changes that are already made in that PR, like adding keyword args instead of positional on some function calls. I agree that more invasive rewrites that involves moving a lot of code around, like making @ffineis can you look at the diff in this PR and let me know if you're ok with me merging it? If you say "please don't, this would be really annoying for me" we can pause this style-only PR until later. |
Ok I've merged the latest |
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.
Re-approving 🙂 .
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. |
When reviewing #3515 , I held back some PR review suggestions about code style. I didn't want to hold up the PR for minor style things.
This PR proposes some style changes that I think will make the Dask code easier to read and change.
lightgbm.dask
module docstring to make it clear that this module originally came fromdask-lightgbm
, which itself originally was based ondask-xgbooost