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

[dask] [python-package] use keyword args for internal function calls #3755

Merged
merged 8 commits into from
Jan 22, 2021

Conversation

jameslamb
Copy link
Collaborator

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.

  • uses only keyword arguments for multi-argument function calls
  • breaks up long function calls into one argument per line, to more easily see parameters and values
  • adds blank line after the end of every method definition
  • updates the lightgbm.dask module docstring to make it clear that this module originally came from dask-lightgbm, which itself originally was based on dask-xgbooost

@StrikerRUS
Copy link
Collaborator

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.

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?

Copy link
Collaborator

@StrikerRUS StrikerRUS left a 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)

python-package/lightgbm/dask.py Outdated Show resolved Hide resolved
Co-authored-by: Nikita Titov <nekit94-08@mail.ru>
@jameslamb
Copy link
Collaborator Author

LGTM except 1 typo!

But pleeeease do not adopt old-fashioned 80-char line length limit !
#2437 (comment)

Thanks!

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?

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 _train() and _predict() methods on _LGBMModel, should not be done until #3708 is merged.

@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.

@jameslamb
Copy link
Collaborator Author

Ok I've merged the latest master into this and resolved conflicts. I'll do that again after I merge #3755, then merge this.

Copy link
Collaborator

@StrikerRUS StrikerRUS left a comment

Choose a reason for hiding this comment

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

Re-approving 🙂 .

@StrikerRUS StrikerRUS merged commit b8fc476 into microsoft:master Jan 22, 2021
@jameslamb jameslamb deleted the misc/dask-style branch January 24, 2021 03:49
@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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants