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] added f-strings to python-package/lightgbm/dask.py #4144

Merged
merged 17 commits into from
May 15, 2021
Merged

[python] added f-strings to python-package/lightgbm/dask.py #4144

merged 17 commits into from
May 15, 2021

Conversation

NovusEdge
Copy link
Contributor

No description provided.

@NovusEdge NovusEdge changed the title added f-string to python-package/lightgbm/dask.py added f-strings to python-package/lightgbm/dask.py Mar 30, 2021
@NovusEdge
Copy link
Contributor Author

#4136

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 very much for your continued efforts! Please see the suggestions I left.

To test changes locally, I recommend that you try installing the library:

cd python-package
python setup.py install

And re-building the documentation (since this PR touches some things called __doc__), following the steps at https://github.com/microsoft/LightGBM/blob/master/docs/README.rst#build.

Building locally might help you to iterate faster and to get reviews more quickly.

python-package/lightgbm/dask.py Outdated Show resolved Hide resolved
python-package/lightgbm/dask.py Outdated Show resolved Hide resolved
python-package/lightgbm/dask.py Outdated Show resolved Hide resolved
python-package/lightgbm/dask.py Outdated Show resolved Hide resolved
python-package/lightgbm/dask.py Outdated Show resolved Hide resolved
python-package/lightgbm/dask.py Outdated Show resolved Hide resolved
python-package/lightgbm/dask.py Outdated Show resolved Hide resolved
python-package/lightgbm/dask.py Outdated Show resolved Hide resolved
python-package/lightgbm/dask.py Outdated Show resolved Hide resolved
@jameslamb jameslamb self-requested a review May 10, 2021 23:09
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.

@NovusEdge @StrikerRUS this PR and the others like it (#4142, #4143, #4144) have been open for a few weeks with no updates, so I'd like to update them and move them forward.

I started with this PR. I've used the power that GitHub gives us as maintainers to push to branches on forks that are involved in open pull requests to this repo. I've pushed commits to this PR, but @NovusEdge you'll still get credit as a contributor when this is merged to master.

I used the commands below to update this branch with the latest master.

git clone --recursive git@github.com:NovusEdge/LightGBM.git novus-lgb
cd novus-lgb

git remote add upstream git@github.com:microsoft/LightGBM.git
git pull origin master
git fetch origin dask
git checkout dask
git merge master
git push origin dask

After that, I built the documentation locally to check that the changes are ok. Docs look correct to me!

cd docs
make clean html
open _build/html/index.html

@jameslamb jameslamb changed the title added f-strings to python-package/lightgbm/dask.py [python] added f-strings to python-package/lightgbm/dask.py May 11, 2021
@StrikerRUS
Copy link
Collaborator

Need to be updated

raise TypeError('Data must be one of: numpy arrays, pandas dataframes, sparse matrices (from scipy). Got %s.' % str(type(seq[0])))

'%s:%d' % (urlparse(worker_address).hostname, port)

raise TypeError('Data must be either Dask Array or Dask DataFrame. Got %s.' % str(type(data)))

@jameslamb
Copy link
Collaborator

Need to be updated

updated in 8f01b98

python-package/lightgbm/dask.py Outdated Show resolved Hide resolved
python-package/lightgbm/dask.py Outdated Show resolved Hide resolved
python-package/lightgbm/dask.py Outdated Show resolved Hide resolved
python-package/lightgbm/dask.py Outdated Show resolved Hide resolved
python-package/lightgbm/dask.py Outdated Show resolved Hide resolved
python-package/lightgbm/dask.py Outdated Show resolved Hide resolved
@jameslamb jameslamb requested a review from StrikerRUS May 13, 2021 03:11
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.

Thanks for helping with this!
Here are some more problems I've found when rendered the docs.

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

Thanks very much @NovusEdge ! And as always thank you for the thorough review, @StrikerRUS

@jameslamb jameslamb merged commit 3a657c8 into microsoft:master May 15, 2021
@StrikerRUS
Copy link
Collaborator

@jameslamb Many thanks for pushing PRs with f-strings forward!

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

3 participants