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

Update breadth_first_search_2.py #7765

Merged
merged 5 commits into from
Oct 28, 2022

Conversation

Cjkjvfnby
Copy link
Contributor

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

@algorithms-keeper algorithms-keeper bot added enhancement This PR modified some existing files awaiting reviews This PR is ready to be reviewed labels Oct 27, 2022
@algorithms-keeper algorithms-keeper bot mentioned this pull request Oct 27, 2022
14 tasks
@cclauss
Copy link
Member

cclauss commented Oct 28, 2022

Let's keep both the existing function and the new function in the same file and let's be sure that they are both tested with the same tests and test data so visitors can compare them side-by-side.

Let's also add a timeit benchmark so visitors can understand the performance differences between the two implementations. This repo has lots of timeit benchmarks... https://github.com/TheAlgorithms/Python/search?q=timeit

@algorithms-keeper algorithms-keeper bot added tests are failing Do not merge until tests pass and removed tests are failing Do not merge until tests pass labels Oct 28, 2022
@Cjkjvfnby
Copy link
Contributor Author

keep both the existing function and the new function in the same file and let's be sure that they are both tested with the same tests and test data so visitors can compare them side-by-side.

I still need to modify the existing function, because it returns traversed elements in the unordered collection.

Both functions use exactly the same approach, just different data structure that implements a similar API.

The not thread-safe deque beats thread-safe Queue in a single-threaded environment.

breadth_first_search                finished 10000 runs in 0.22710 seconds
breadth_first_search_with_deque     finished 10000 runs in 0.01708 seconds

@cclauss
Copy link
Member

cclauss commented Oct 28, 2022

I still need to modify the existing function, because it returns traversed elements in the unordered collection.

I do not understand this. Why should the new function force the old function to change? I would think that traversing elements in an unordered collection was the original problem.

The benchmark looks awesome. Please put the benchmark results in the comments so that the reader has a sense of with is faster and by how much.

@Cjkjvfnby
Copy link
Contributor Author

Why should the new function force the old function to change?

BFS is an algorithm to traverse the tree in a specific order (gif from wiki). So 2 items are important here that all nodes are visited and the order in which they are visited. The problem with the existing implementation is that it returns all nodes in the collection that does not maintain order and remove duplicates (set). And the test was checking only that all nodes have been visited at least once. I consider this to be a very strange return type for the BFS and I am willing to address it.

Another problem is that implementation of the queue data structure that is used in the first function is not really efficient for such tasks. It's like hammering nails with a microscope. I'd prefer o keep only one implementation with a more suitable data structure.

@cclauss
Copy link
Member

cclauss commented Oct 28, 2022

Maintaining the breadth-first order is vital but the wiki definition says nothing about eliminating duplicate values.

Having only a single implementation is not an option.

@Cjkjvfnby
Copy link
Contributor Author

Maintaining the breadth-first order is vital but the wiki definition says nothing about eliminating duplicate values.

Yes, because these algorithm does not suppose to have any duplication during traversal. Using set as a return value does not allow testing this.

Having only a single implementation is not an option.

Do not worry about it, there are at least 5 files related to it in the repository.

These two function does not bring any diversity to the implementation, they are pretty much the same. I could keep both implementations, but using Queue here is a bad example.

@algorithms-keeper algorithms-keeper bot removed the awaiting reviews This PR is ready to be reviewed label Oct 28, 2022
@cclauss cclauss merged commit 762afc0 into TheAlgorithms:master Oct 28, 2022
@Cjkjvfnby Cjkjvfnby deleted the improve-bfs-code branch October 28, 2022 21:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This PR modified some existing files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants