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] use f-strings for concatenation in examples/python-guide/logistic_regression.py #4356

Merged
merged 5 commits into from
Jun 8, 2021

Conversation

sagnik1511
Copy link
Contributor

@sagnik1511 sagnik1511 commented Jun 8, 2021

This PR contributes to issue #4136 .
I've updated the file as I stated in #4136 (comment).

In this PR I have updated the file .\examples\python-guide\logistic_regression.py where I have updated 4 lines of code with the f-string format.

I have previously updated 2 lines and updated two lines after your feedback @jameslamb.
Do let me know if there's an issue. Thank You.

@sagnik1511 sagnik1511 changed the title [python] Migrated to f-strings for ..nuget\create_nuget.py #4136 @sagnik1511 (#4136) [python] Migrated to f-strings for .\examples\python-guide\logistic_regression.py #4136 @sagnik1511 (#4136) Jun 8, 2021
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! Please see my newest comments.


A few other notes, since I think you might be new to creating pull requests on GitHub.

  1. You do not need to (and should not) close a pull request and open a new one in response to review comments. Once you have opened a pull request from a branch, every commit you push to that branch will get added to the pull request for as long as the pull request is open.
  2. I've changed the title for this pull request back to the one I originally had written in [python-package] use f-strings for concatenation in examples/python-guide/logistic_regression.py #4355. It is not necessary to include your username in the title, and we'd prefer that you did not. I think you may have been confused looking at the changelog link I included in https://github.com/microsoft/LightGBM/pull/4355#pullrequestreview-678755908...the username and PR number are added automatically to that changelog. I recommend that you look through some previous PRs in this project to get some ideas about how titles are usually written: https://github.com/microsoft/LightGBM/pulls?q=is%3Apr+is%3Aclosed
  3. You should not use language like "fixes" or "resolves" an issue in your PR title or comments unless you are fully fixing it. I removed the word "resolves" from the PR description, because if it was left then GitHub would automatically close Migrate to f-strings for the Python code #4136 when this PR was merged. We don't want that in this case, since this is only a partial step toward that issue.

examples/python-guide/logistic_regression.py Outdated Show resolved Hide resolved
Comment on lines 112 to 113
print(f"Best `binary` time: {str(min(A))}")
print(f"Best `xentropy` time: {str(min(B))}")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
print(f"Best `binary` time: {str(min(A))}")
print(f"Best `xentropy` time: {str(min(B))}")
print(f"Best `binary` time: {min(A)}")
print(f"Best `xentropy` time: {min(B)}")

str() is no longer necessary inside an f-string, so I think this can be simplified.

examples/python-guide/logistic_regression.py Outdated Show resolved Hide resolved
@jameslamb jameslamb changed the title [python] Migrated to f-strings for .\examples\python-guide\logistic_regression.py #4136 @sagnik1511 (#4136) [python-package] use f-strings for concatenation in examples/python-guide/logistic_regression.py Jun 8, 2021
@sagnik1511
Copy link
Contributor Author

I have copied the whole file and changed that into this
https://colab.research.google.com/drive/1Gnf41gU5c6ZP8rDajMexF4CWepr1D5vi?usp=sharing
Can you tell me if this is okay or not??

@jameslamb
Copy link
Collaborator

jameslamb commented Jun 8, 2021

I have copied the whole file and changed that into this
https://colab.research.google.com/drive/1Gnf41gU5c6ZP8rDajMexF4CWepr1D5vi?usp=sharing
Can you tell me if this is okay or not??

Sorry, I am not going to click that. It's a security risk for us as maintainers to click links from people we don't know.

This project uses GitHub to manage code review, so please push any changes you'd like to make to this pull request and we'd be happy to review them here.

@sagnik1511
Copy link
Contributor Author

I have copied the whole file and changed that into this
https://colab.research.google.com/drive/1Gnf41gU5c6ZP8rDajMexF4CWepr1D5vi?usp=sharing
Can you tell me if this is okay or not??

Sorry, I am not going to click that. It's security risk for us as maintainers to click links from people we don't know.

This project uses GitHub to manage code review, so please push any changes you'd like to make to this pull request and we'd be happy to review them here.

Okay, sir, I've updated the file in your required format. Should I create a new PR then??

@jameslamb
Copy link
Collaborator

Okay, sir, I've updated the file in your required format. Should I create a new PR then??

I can see that you've pushed changes to examples/python-guide/logistic_regression.py to this pull request. So I'm confused, what other file are you referring to?

@sagnik1511
Copy link
Contributor Author

Okay, sir, I've updated the file in your required format. Should I create a new PR then??

I can see that you've pushed changes to examples/python-guide/logistic_regression.py to this pull request. So I'm confused, what other file are you referring to?

Sorry, Sir, I was confused a bit.
I think now the PR is in the required format. So, this can be merged, right?

@jameslamb
Copy link
Collaborator

So, this can be merged, right?

Maybe. A set of automated checks runs on every commit to this project. You can see them near the bottom of the PR.

image

At a minimum, all of these will need to pass before the PR is merged.

@sagnik1511
Copy link
Contributor Author

So, this can be merged, right?

Maybe. A set of automated checks runs on every commit to this project. You can see them near the bottom of the PR.

image

At a minimum, all of these will need to pass before the PR is merged.

Okay, Sir. So now I should wait for the completion of those reviews, right? Or are there any changes left from my side?
And also sorry for bothering you this much :(

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, thanks!

@jameslamb jameslamb self-requested a review June 8, 2021 22:19
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 good, thank you very much for the help!

@jameslamb jameslamb merged commit 8e5079e into microsoft:master Jun 8, 2021
@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