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

Added Kruskal's Algorithm (more organized than the one present) #2218

Merged
merged 8 commits into from Aug 12, 2020

Conversation

@ruppysuppy
Copy link
Contributor

ruppysuppy commented Jul 21, 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}.
# create a new set with x as its member
self.map[x] = Disjoint_Set_Tree_Node(x)

def find_set(self, x):
def find_set(self, x: int) -> Disjoint_Set_Tree_Node:

This comment has been minimized.

@ruppysuppy

ruppysuppy Jul 21, 2020

Author Contributor

Missed the type hints in a few places, will add them

@ruppysuppy
Copy link
Contributor Author

ruppysuppy commented Jul 21, 2020

Could you review the PR please, @cclauss?

if self.map[x] != self.map[x].parent:
self.map[x].parent = self.find_set(self.map[x].parent.key)
return self.map[x].parent
Comment on lines 24 to 26

This comment has been minimized.

@cclauss

cclauss Aug 11, 2020

Member
Suggested change
if self.map[x] != self.map[x].parent:
self.map[x].parent = self.find_set(self.map[x].parent.key)
return self.map[x].parent
self_map_x = self.map[x]
if self_map_x != self_map_x.parent:
self_map_x.parent = self.find_set(self_map_x.parent.key)
return self_map_x.parent

OPTIONAL: You might gain a bit of performance...
https://wiki.python.org/moin/PythonSpeed/PerformanceTips#Loops

This comment has been minimized.

@ruppysuppy

ruppysuppy Aug 12, 2020

Author Contributor

I'm keeping it as list comprehension, converting

[disjoint_set.make_set(node) for node in self.connections]

to

map(disjoint_set.make_set, self.connections.keys())

yields:

**********************************************************************
File "d:/Desktop/Projects/GitUploads/Python/graphs/minimum_spanning_tree_kruskal2.py", line 77, in __main__.GraphUndirectedWeighted.kruskal
Failed example:
    mst = graph.kruskal()
Exception raised:
    Traceback (most recent call last):
      File "C:\Program Files\Python37\lib\doctest.py", line 1329, in __run
        compileflags, 1), test.globs)
      File "<doctest __main__.GraphUndirectedWeighted.kruskal[7]>", line 1, in <module>
        mst = graph.kruskal()
      File "d:/Desktop/Projects/GitUploads/Python/graphs/minimum_spanning_tree_kruskal2.py", line 100, in kruskal
        parentu = disjoint_set.find_set(u)
      File "d:/Desktop/Projects/GitUploads/Python/graphs/minimum_spanning_tree_kruskal2.py", line 24, in find_set
        elem_ref = self.map[x]
    KeyError: 1
**********************************************************************
File "d:/Desktop/Projects/GitUploads/Python/graphs/minimum_spanning_tree_kruskal2.py", line 78, in __main__.GraphUndirectedWeighted.kruskal
Failed example:
    assert 5 not in mst.connections[3]
Exception raised:
    Traceback (most recent call last):
      File "C:\Program Files\Python37\lib\doctest.py", line 1329, in __run
        compileflags, 1), test.globs)
      File "<doctest __main__.GraphUndirectedWeighted.kruskal[8]>", line 1, in <module>
        assert 5 not in mst.connections[3]
    NameError: name 'mst' is not defined
**********************************************************************
1 items had failures:
   2 of   9 in __main__.GraphUndirectedWeighted.kruskal
***Test Failed*** 2 failures.
Copy link
Member

cclauss left a comment

Cool!!

Copy link
Contributor Author

ruppysuppy left a comment

The naming convention is a bit a bit off

@ruppysuppy ruppysuppy force-pushed the ruppysuppy:kruskal2 branch from 2ca84d9 to 3073d08 Aug 12, 2020
# add a node ONLY if its not present in the graph
if node not in self.connections:
self.connections[node] = {}
self.nodes += 1

This comment has been minimized.

@cclauss

cclauss Aug 12, 2020

Member

Under what circumstances is self.nodes != len(self.connections)?

This comment has been minimized.

@ruppysuppy

ruppysuppy Aug 12, 2020

Author Contributor

Yeah its always same. Should I remove it?

This comment has been minimized.

@cclauss

cclauss Aug 12, 2020

Member

Your call. You already have my approval so you can squash & merge whenever you want (as long as the tests are green). Well done!

This comment has been minimized.

@ruppysuppy

ruppysuppy Aug 12, 2020

Author Contributor

I have made the changes, but I cannot merge since I'm not a member

@ruppysuppy ruppysuppy force-pushed the ruppysuppy:kruskal2 branch from 18c1d6b to 05e92ef Aug 12, 2020
github-actions github-actions
@cclauss cclauss merged commit aa46639 into TheAlgorithms:master Aug 12, 2020
1 check passed
1 check passed
Travis CI - Pull Request Build Passed
Details
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

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