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 make_root in DSU #342

Merged
merged 3 commits into from
Mar 13, 2021
Merged

Added make_root in DSU #342

merged 3 commits into from
Mar 13, 2021

Conversation

Smit-create
Copy link
Member

References to other Issues or PRs or Relevant literature

Brief description of what is fixed or changed

Added a method to modify the parent of set to which key belongs

>>> dst = DisjointSetForest()
>>> dst.make_set(1)
>>> dst.make_set(2)
>>> dst.union(1, 2)
>>> dst.find_root(2).key
1
>>> dst.make_root(2)
>>> dst.find_root(2).key
2

@codecov
Copy link

codecov bot commented Mar 10, 2021

Codecov Report

Merging #342 (0f8e381) into master (a21d00e) will increase coverage by 0.006%.
The diff coverage is 100.000%.

@@              Coverage Diff              @@
##            master      #342       +/-   ##
=============================================
+ Coverage   98.550%   98.556%   +0.006%     
=============================================
  Files           25        25               
  Lines         3243      3257       +14     
=============================================
+ Hits          3196      3210       +14     
  Misses          47        47               
Impacted Files Coverage Δ
...ucts/miscellaneous_data_structures/disjoint_set.py 100.000% <100.000%> (ø)

Impacted file tree graph

@Smit-create
Copy link
Member Author

@czgdp1807 I was thinking if we could change the implementation of the current DSU from maps to arrays(take n(size of array) as an argument while creating the instance) as it is costing an additional factor O(logN) in time complexity

@czgdp1807
Copy link
Member

In Python dict is a hashing based data structure It's counterpart in C++ is unordered_map. map in C++ is based on red-black tree and is not hashing based hence, it costs O(lg N) overhead.

@@ -74,3 +77,18 @@ def union(self, key1, key2):

y_root.parent = x_root
x_root.size += y_root.size

def make_root(self, key):
Copy link
Member

Choose a reason for hiding this comment

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

The feature looks good. Could you share an example (book/lecture notes examples will work) where this is used/required?

Copy link
Member Author

Choose a reason for hiding this comment

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

Presently, I don't remember the exact the example but, we can consider this example https://cp-algorithms.com/data_structures/disjoint_set_union.html#toc-tgt-10 wherein we generally make root which Is largest element in set

Copy link
Member

Choose a reason for hiding this comment

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

I see. That's great. Could you solve one such problem using DSU API in pydatastructs and show the code here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I will trying finding the exact problem I solved using this method and show that

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, this is the problem where I used the above method: https://codeforces.com/contest/1133/problem/F2, and the submission of mine can be found at: https://codeforces.com/contest/1133/submission/109674863

@Smit-create
Copy link
Member Author

@czgdp1807 Looks good to go?

if current_root.key != key:
key_set = self.tree[key]
current_root.parent = key_set
key_set.size = current_root.size
Copy link
Member

Choose a reason for hiding this comment

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

I think the size of current_root should change. Just take the following example,

>>> from pydatastructs import *
>>> dst = DisjointSetForest()
>>> dst.make_set(1)
>>> dst.make_set(2)
>>> dst.make_set(3)
>>> dst.tree[1].size
1
>>> dst.tree[2].size
1
>>> dst.tree[3].size
1
>>> dst.union(2, 3)
>>> dst.tree[1].size
1
>>> dst.tree[2].size
2
>>> dst.tree[3].size
1
>>> dst.make_root(3)
>>> dst.tree[1].size
1
>>> dst.tree[2].size # should have been 1 because now 2 is pointing to 3
2
>>> dst.tree[3].size
2

I think this inconsistency can cause problems when root is shifted a lot of times to different elements of set. We should keep things correct wherever possible.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this inconsistency can cause problems when root is shifted a lot of times to different elements of set. We should keep things correct wherever possible.

This can be recitifed using re-rooting technique: https://wiki.algo.is/Rerooting%20technique. Actually while changing the root I didn't tried to maintain tree/subtree structure and just focused on the size of total elements belonging to that set. However using above technique we can re-root by maintaing tree/subtree structure too

Copy link
Member

Choose a reason for hiding this comment

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

I think we should have it. We may need to track the nodes whose size will change due to change of root. It may or not be that tricky. Try to do it by just using the parent pointers.

@Smit-create Smit-create requested a review from czgdp1807 March 13, 2021 04:20
@czgdp1807
Copy link
Member

LGTM. Merging. Thanks.

@czgdp1807 czgdp1807 merged commit 6897169 into codezonediitj:master Mar 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants