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

Possible improvement to path compression runtime #321

Open
gnat79 opened this issue Sep 13, 2019 · 0 comments · May be fixed by #415
Open

Possible improvement to path compression runtime #321

gnat79 opened this issue Sep 13, 2019 · 0 comments · May be fixed by #415

Comments

@gnat79
Copy link

gnat79 commented Sep 13, 2019

p, self.parent_arr[p] = self.parent_arr[p], n

I thinks the intention here is to implement path compression, but it could be that this code only does 1/k of the total optimization because of the way the tuple assignment works. Here "k" would be the number of hops from this node up to the root. If I'm right, this would make a big difference for large datasets where the MST is fairly large.

If I understand python correctly (debatable!), the right side tuple is evaluated from left to right, and then the left side assignments are made from left to right. That means that means that p on the left is updated before self.parent[p] is updated, which I do not believe is the intended behavior.

I think the following code will work correctly. I can probably fork and test this code over the weekend, but hoping Leland or another author can maybe clarify before I dig in too much.

        while self.parent_arr[p] != n:
            #p, self.parent_arr[p] = self.parent_arr[p], n
            tmp = self.parent_arr[p]
            self.parent_arr[p] = n
            p = tmp
        return n

Thanks in advance for spending time looking at this!

markopy added a commit to markopy/hdbscan that referenced this issue Oct 3, 2020
markopy added a commit to markopy/hdbscan that referenced this issue Oct 3, 2020
@markopy markopy linked a pull request Oct 3, 2020 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant