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

Changes data type from set to list for correct return result #1816

Open
wants to merge 1 commit into
base: master
from

Conversation

@b0r1sp
Copy link

b0r1sp commented Mar 26, 2020

It seems, saving 'explored' in a set returns wrong results. During traversal, the order of the entries in var 'explored' are changed by python (by version 2 AND 3).

Result during iteration with data type 'set':

set(['A', 'B'])
set(['A', 'C', 'B'])
set(['A', 'C', 'B', 'D'])
set(['A', 'C', 'B', 'E', 'D'])
set(['A', 'C', 'B', 'E', 'D', 'F'])
set(['A', 'C', 'B', 'E', 'D', 'F'])

For comparison, here's the result with data type 'list':

['A', 'B']
['A', 'B', 'C']
['A', 'B', 'C', 'D']
['A', 'B', 'C', 'D', 'E']
['A', 'B', 'C', 'D', 'E', 'F']
['A', 'B', 'C', 'D', 'E', 'F']

The result is for the old example-graph essentially the same, but for more complex graphs it will be wrong.

I added a slightly more complex graph:

G = {
    "A": ["B", "E", "C"],
    "B": ["A", "F", "C"],
    "C": ["A", "B", "E", "I", "H"],
    "D": ["F"],
    "E": ["A", "C"],
    "F": ["G", "D", "B"],
    "G": ["F"],
    "H": ["I", "C"],
    "I": ["H", "C"]
}

Here's the wrong result with data type 'set' - note how the order changes:

set(['A', 'B'])
set(['A', 'B', 'E'])
set(['A', 'C', 'B', 'E'])
set(['A', 'C', 'B', 'E', 'F'])
set(['A', 'C', 'B', 'E', 'F', 'I'])
set(['A', 'C', 'B', 'E', 'F', 'I', 'H'])
set(['A', 'C', 'B', 'E', 'G', 'F', 'I', 'H'])
set(['A', 'C', 'B', 'E', 'D', 'G', 'F', 'I', 'H'])
set(['A', 'C', 'B', 'E', 'D', 'G', 'F', 'I', 'H'])

What should be the result? (order of vertices on every level does not matter)

Level 0: A
Level 1: B,C,E
Level 2: F,I,H
Level 3: G,D

If you take a close look above, e.g. D is returned on the wrong level.

Here's the correct result with data type 'list':

['A', 'B']
['A', 'B', 'E']
['A', 'B', 'E', 'C']
['A', 'B', 'E', 'C', 'F']
['A', 'B', 'E', 'C', 'F', 'I']
['A', 'B', 'E', 'C', 'F', 'I', 'H']
['A', 'B', 'E', 'C', 'F', 'I', 'H', 'G']
['A', 'B', 'E', 'C', 'F', 'I', 'H', 'G', 'D']
['A', 'B', 'E', 'C', 'F', 'I', 'H', 'G', 'D']

I suggest to include the more complex graph for testing purposes.

  • Add an algorithm?
  • [x ] Fix a bug or typo in an existing algorithm?
  • Documentation change?
@b0r1sp
Copy link
Author

b0r1sp commented Mar 27, 2020

I added a new graph, so the test needs to be fixed accordingly.

@@ -31,13 +34,12 @@ def bfs(graph, start):
>>> ''.join(sorted(bfs(G, 'A')))
'ABCDEF'

This comment has been minimized.

Copy link
@cclauss

cclauss Mar 27, 2020

Member
Suggested change
'ABCDEF'
'ABCDEFGHI'

This comment has been minimized.

Copy link
@b0r1sp

b0r1sp Mar 27, 2020

Author

IMHO:

    >>> ''.join(bfs(G, 'A'))
    'ABECFIHGD'

The sorting is important.

This comment has been minimized.

Copy link
@cclauss

cclauss Mar 27, 2020

Member

I added a new graph, so the test function needs to be fixed accordingly because currently it returns ABCDEFGHI not ABECFIHGD. This is why Travis CI is failing this pull request. Either the test must be fixed to match the function output or the function must be fixed to match the test's expected result. They must agree for this pull request to be considered.

This comment has been minimized.

Copy link
@cclauss

cclauss Mar 27, 2020

Member

Python sets are defined to be an unordered collection with no duplicate elements. This means that they should not be used if the order of elements is important.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.