The Wayback Machine - https://web.archive.org/web/20200914220645/https://github.com/TheAlgorithms/Python/pull/2410
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

Optimized recursive_bubble_sort #2410

Merged
merged 4 commits into from Sep 10, 2020
Merged

Optimized recursive_bubble_sort #2410

merged 4 commits into from Sep 10, 2020

Conversation

@shellhub
Copy link
Member

shellhub commented Sep 8, 2020

Describe your change:

  • Add an algorithm?
  • Fix a bug or typo in an existing algorithm?
  • Documentation change?

Checklist:

  • I have read CONTRIBUTING.md.
  • This pull request is all my own work -- I have not plagiarized.
  • I know that pull requests will not be merged if they fail the automated tests.
  • This PR only changes one algorithm file. To ease review, please open separate PRs for separate algorithms.
  • All new Python files are placed inside an existing directory.
  • All filenames are in all lowercase characters with no spaces or dashes.
  • All functions and variable names follow Python naming conventions.
  • All function parameters and return values are annotated with Python type hints.
  • All functions have doctests that pass the automated testing.
  • All new algorithms have a URL in its comments that points to Wikipedia or other similar explanation.
  • If this pull request resolves one or more open issues then the commit message contains Fixes: #{$ISSUE_NO}.
@shellhub shellhub requested a review from cclauss Sep 8, 2020
@shellhub
Copy link
Member Author

shellhub commented Sep 8, 2020

@cclauss Please review. Thanks in advance :)

sorts/recursive_bubble_sort.py Outdated Show resolved Hide resolved
sorts/recursive_bubble_sort.py Outdated Show resolved Hide resolved
Copy link
Member Author

shellhub left a comment

@cclauss fixed

sorts/recursive_bubble_sort.py Outdated Show resolved Hide resolved
@TravisBuddy
Copy link

TravisBuddy commented Sep 8, 2020

Hey @shellhub,
Something went wrong with the build.

TravisCI finished with status errored, which means the build failed because of something unrelated to the tests, such as a problem with a dependency or the build process itself.

View build log

TravisBuddy Request Identifier: cb2f0a90-f1af-11ea-ba41-b1d3a642fad8
@TravisBuddy
Copy link

TravisBuddy commented Sep 8, 2020

Hey @shellhub,
Something went wrong with the build.

TravisCI finished with status errored, which means the build failed because of something unrelated to the tests, such as a problem with a dependency or the build process itself.

View build log

TravisBuddy Request Identifier: ec7590c0-f1af-11ea-ba41-b1d3a642fad8
@TravisBuddy
Copy link

TravisBuddy commented Sep 8, 2020

Hey @shellhub,
Something went wrong with the build.

TravisCI finished with status errored, which means the build failed because of something unrelated to the tests, such as a problem with a dependency or the build process itself.

View build log

TravisBuddy Request Identifier: 18cf7a50-f1b0-11ea-ba41-b1d3a642fad8
@TravisBuddy
Copy link

TravisBuddy commented Sep 8, 2020

Hey @shellhub,
Something went wrong with the build.

TravisCI finished with status errored, which means the build failed because of something unrelated to the tests, such as a problem with a dependency or the build process itself.

View build log

TravisBuddy Request Identifier: 3c4e1630-f1b0-11ea-ba41-b1d3a642fad8
Copy link
Member Author

shellhub left a comment

fix build error

sorts/recursive_bubble_sort.py Outdated Show resolved Hide resolved
@TravisBuddy
Copy link

TravisBuddy commented Sep 8, 2020

Travis tests have failed

Hey @shellhub,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

TravisBuddy Request Identifier: 97257ab0-f1b2-11ea-ba41-b1d3a642fad8
@shellhub
Copy link
Member Author

shellhub commented Sep 8, 2020

@cclauss why Travic CI occur error?

@shellhub shellhub force-pushed the shellhub:dev branch from f07072c to 2528bcb Sep 8, 2020
@github-actions github-actions bot force-pushed the shellhub:dev branch from 986da35 to cd8f05f Sep 8, 2020
@TravisBuddy
Copy link

TravisBuddy commented Sep 8, 2020

Travis tests have failed

Hey @shellhub,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

TravisBuddy Request Identifier: d29c1380-f1b5-11ea-ba41-b1d3a642fad8
@cclauss
Copy link
Member

cclauss commented Sep 8, 2020

Probably trailing whitespace deleted in doctests.

@shellhub
Copy link
Member Author

shellhub commented Sep 8, 2020

How to solve?😭

@cclauss
Copy link
Member

cclauss commented Sep 8, 2020

See which files are failing in Travis CI and then undo the changes to those files.

@shellhub
Copy link
Member Author

shellhub commented Sep 8, 2020

Yes. I found why build error was occur. Because whitespace deleted in doctests. Should I update origin source code and commit again? have you any better suggestion? @cclauss
image

@shellhub shellhub force-pushed the shellhub:dev branch from cd8f05f to d991854 Sep 8, 2020
@github-actions github-actions bot force-pushed the shellhub:dev branch from 66fe48c to b538271 Sep 8, 2020
@TravisBuddy
Copy link

TravisBuddy commented Sep 8, 2020

Travis tests have failed

Hey @shellhub,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

TravisBuddy Request Identifier: 5134a1b0-f1e5-11ea-ba41-b1d3a642fad8
@shellhub shellhub force-pushed the shellhub:dev branch from b538271 to 0232a35 Sep 8, 2020
@github-actions github-actions bot force-pushed the shellhub:dev branch from 94e5def to 3fcf98c Sep 8, 2020
@TravisBuddy
Copy link

TravisBuddy commented Sep 8, 2020

Travis tests have failed

Hey @shellhub,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

TravisBuddy Request Identifier: ece4a470-f1ea-11ea-ba41-b1d3a642fad8
@shellhub shellhub force-pushed the shellhub:dev branch from 3fcf98c to 2528bcb Sep 8, 2020
@github-actions github-actions bot force-pushed the shellhub:dev branch from 4a8f92d to 777f40c Sep 8, 2020
@TravisBuddy
Copy link

TravisBuddy commented Sep 8, 2020

Travis tests have failed

Hey @shellhub,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

TravisBuddy Request Identifier: 58945cd0-f1ee-11ea-ba41-b1d3a642fad8
@spamegg1
Copy link
Contributor

spamegg1 commented Sep 9, 2020

The build log is very difficult to understand.
It's showing all kinds of errors from OTHER files.
What is going on?
Also it says above "60 files changed" in shellhub:dev branch, was this your intention @shellhub ?
I suppose this was caused by the forced push that applied psf/black to everything?
It might be best to just start over with 1 file changed.

@cclauss
Copy link
Member

cclauss commented Sep 9, 2020

This was more than just running black. In the cases of the failing doctests, whitespace has been removed from the end of lines which the doctest needs to see for an exact match. This is one of the reasons why black will not suppress trailing whitespace that appears inside of triple quoted strings. There are three files that fail the Travis doctests. If the changes to those three files are reverted then the tests will pass again.

Copy link
Member Author

shellhub left a comment

Added whitespace

traversals/binary_tree_traversals.py Outdated Show resolved Hide resolved
@github-actions github-actions bot force-pushed the shellhub:dev branch from 2ddb1d6 to 8d59990 Sep 9, 2020
@TravisBuddy
Copy link

TravisBuddy commented Sep 9, 2020

Travis tests have failed

Hey @shellhub,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

TravisBuddy Request Identifier: c60c3520-f26c-11ea-82b8-f1691ab9f797
@shellhub shellhub force-pushed the shellhub:dev branch from 8d59990 to 13952c9 Sep 9, 2020
@github-actions github-actions bot force-pushed the shellhub:dev branch from d1c4d94 to 0fa3585 Sep 9, 2020
@cclauss
Copy link
Member

cclauss commented Sep 9, 2020

My sense is that you add the trailing whitespace and then the autoblack GitHub Action undoes it so the tests fail.

@shellhub
Copy link
Member Author

shellhub commented Sep 9, 2020

@cclauss All checks have passed. Can you review again. Thanks :)

@cclauss
Copy link
Member

cclauss commented Sep 9, 2020

I am completely lost by the change to bubble sort! We now require the user to provide length but our algorithm never uses that variable.

@shellhub
Copy link
Member Author

shellhub commented Sep 9, 2020

I am completely lost by the change to bubble sort! We now require the user to provide length but our algorithm never uses that variable.

Not exactly. length is optional to user. And we used length in our recursion code.

return list_data if not swapped else bubble_sort(list_data, length - 1)
@shellhub
Copy link
Member Author

shellhub commented Sep 9, 2020

Get a maximum value to the end of list for each recursive call. Then recursion sort left elements (length - 1 elements)

@cclauss
Copy link
Member

cclauss commented Sep 9, 2020

Please look at it again. Passing a number around but not actually using it (outside of decrementing it and passing it back to yourself) is a waste of cycles.

@shellhub shellhub force-pushed the shellhub:dev branch from 0fa3585 to b0acd82 Sep 9, 2020
@github-actions github-actions bot force-pushed the shellhub:dev branch from 77a9492 to eaa5c9c Sep 9, 2020
@shellhub
Copy link
Member Author

shellhub commented Sep 9, 2020

@cclauss you are right. I forget make changes that you mentioned before. I make changes like this. Please review again. Thanks :)

    length = length or len(list_data)
    swapped = False
    for i in range(length - 1):
        if list_data[i] > list_data[i + 1]:
            list_data[i], list_data[i + 1] = list_data[i + 1], list_data[i]
            swapped = True

    return list_data if not swapped else bubble_sort(list_data, length - 1)
@shellhub
Copy link
Member Author

shellhub commented Sep 9, 2020

I changed for i in range(len(list_data) - 1): to for i in range(length - 1):

@shellhub shellhub requested a review from cclauss Sep 10, 2020
@cclauss
Copy link
Member

cclauss commented Sep 10, 2020

OK... Here is what we are going to do. I will land this now. From now on, no PRs that touch so many files.

Please create a new PR for bubble sort that has both the old algorithm and the "oprimized" version in a single file with a timeit (or similar) benchmark that measures the relative performance of each.

Copy link
Member

cclauss left a comment

LGTM

@cclauss cclauss merged commit 4d0a8f2 into TheAlgorithms:master Sep 10, 2020
1 check passed
1 check passed
Travis CI - Pull Request Build Passed
Details
@shellhub shellhub deleted the shellhub:dev branch Sep 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.