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

Increased efficiency of UnionFind.__getitem__ slightly. #4810

Closed
wants to merge 1 commit into from

Conversation

simonsteinberg
Copy link

The last element in path is the root itself.

@boothby
Copy link
Contributor

boothby commented May 20, 2021

Are you sure this increases efficiency? A microbenchmark tells me otherwise.

In [1]: def original(parents, path): 
   ...:     for node in path: 
   ...:         parents[node] = None 
   ...: def improved(parents, path): 
   ...:     for node in path[:-1]: 
   ...:         parents[node] = None 
   ...: path = list(range(10)) 
   ...: parents = {} 
   ...: print("original:") 
   ...: %timeit original(parents, path) 
   ...: print("improved:") 
   ...: %timeit improved(parents, path) 
   ...: print()                                                                                          
original:
623 ns ± 18 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)
improved:
701 ns ± 31.7 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)

It should be a tiny bit faster to call path.pop or avoid appending the root in the first place

@dschult
Copy link
Member

dschult commented Jun 12, 2021

Yes, it looks like the faster method is to construct the path without the last node.
Something like:

        path = []
        root = self.parents[object]
        while root != object:
            path.append(object)
            object = root
            root = self.parents[object]
        # Note that root is not added to the path

@rossbar
Copy link
Contributor

rossbar commented Dec 12, 2021

Gentle ping @simonsteinberg - do you want to give the above suggestion a try? No worries if you don't have time - we can always convert this to an issue to be picked up later!

@MridulS
Copy link
Member

MridulS commented Jul 5, 2022

Thanks for opening this @simonsteinberg, I've created a new PR with the suggested change in #5844. I'll go ahead and close this :)

@MridulS MridulS closed this Jul 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

5 participants