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

Link/Cut-Trees implementation #186

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from
Open

Conversation

yi-ji
Copy link
Contributor

@yi-ji yi-ji commented Sep 28, 2019

This PR implements Link/Cut-Trees data structure for Boost Graph Library. Since disjoint_sets algorithms has been moved into boost/graph (#169), this should also be the right place to put link_cut_trees algorithm.

This implementation of link_cut_trees supports following operations:

  • make_tree(x): create a new link/cut-tree containing only root x
  • link(x, y): make the tree rooted at x a subtree of y;
  • cut(x): remove the edge connecting x to its parent;
  • find_root(x): find the root of the tree containing x;
  • lowest_common_ancestor(x, y): find the lowest common ancestor of such x and y that have same root.

All operations above are of amortized time complexity O(log n), except O(1) for make_tree.

This implementation refers to:
Robert Endre Tarjan, Data Structures and Network Algorithms, Chapter 5

@jeremy-murphy
Copy link
Contributor

I'm not sure that Boost.Graph really is the right place for disjoint_sets, but it definitely is the right place for link_cut_tree!
I just have one question: how is the documentation going? :)

@yi-ji yi-ji changed the title Link-Cut-Tree implementation Link/Cut-Trees implementation Oct 6, 2019
@yi-ji
Copy link
Contributor Author

yi-ji commented Oct 9, 2019

@jeremy-murphy comments resolved, checks passed :)

@yi-ji
Copy link
Contributor Author

yi-ji commented Jun 11, 2020

Hi @jeremy-murphy Thanks for reviewing! This PR has been ready for quite a while. Any concerns before merging it? Would appreciate someone with write access to have a look :) @jzmaddock

@jeremy-murphy
Copy link
Contributor

Hey, sorry, I don't have much time for open-source projects now that I have a baby! I'll try to review again quickly.

@yi-ji
Copy link
Contributor Author

yi-ji commented Jun 21, 2020

@jeremy-murphy Thanks for helping review again 😄
I've replied or made changes accordingly.
Enjoy parenting! 🍼

@yi-ji
Copy link
Contributor Author

yi-ji commented Jun 24, 2020

Hi @jeremy-murphy good sir, all comments resolved & checks passed, ready again :)

Hope this is not disturbing you @jzmaddock Would you mind having a look to help merge it? :))

@jeremy-murphy
Copy link
Contributor

I can't merge it, sorry, I don't have that access. @jzmaddock is the only person I'm aware of with that access and is active at the moment.

@yi-ji
Copy link
Contributor Author

yi-ji commented Mar 9, 2021

This has been lying in backlog for quite a while. @jzmaddock would you mind having a look? :)

@xiuliren
Copy link

thanks for adding this!
It seems that this data structure could not be serialized using Boost: Serialization?

@yi-ji
Copy link
Contributor Author

yi-ji commented Jun 13, 2021

@jingpengw I guess it depends on the template parameters? All I know is that boost serialization supports POD and STL containers, I think you can use them to instantiate link-cut trees.

@yi-ji
Copy link
Contributor Author

yi-ji commented Aug 1, 2021

@jzmaddock Could you please take a look at this MR when you've got time? It's ready for merge :) Thank you.

@xiuliren
Copy link

@jzmaddock did you get a chance to take a look? is it good to merge?

@jeremy-murphy
Copy link
Contributor

I need to get the CI fixed and I'm going to prioritize merging some bug fixes, but then one day I'll get back to you! ;)

@xiuliren
Copy link

any chance to merge this?

@jeremy-murphy
Copy link
Contributor

any chance to merge this?

Yes, there is a chance. :) I'll try to look over it again soon, but I can't guarantee anything.

@jeremy-murphy
Copy link
Contributor

any chance to merge this?

Yes, there is a chance. :) I'll try to look over it again soon, but I can't guarantee anything.

Btw, what I meant is, I definitely want to merge it, it's just a matter of chance that I find time to do it. :)


namespace boost
{
template<class ElementParentMap, class ElementChildMap = ElementParentMap>
Copy link
Contributor

Choose a reason for hiding this comment

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

I presume that Element in the Parent and Child maps have to be the same type, right? Or at the very minimum, they must be convertible to each other? The reason I ask is that I presume we can simplify the member functions from being templated on Element to being non-template functions. If we impose the strict requirement of being the same type then we can just get the element type from one of the map types, something like:

using Element = ElementParentMap::key_type;

Or if we want to allow more flexibility, then it might have to be:

using Element = typename std::common_type_t<ElementParentMap::key_type, ElementChildMap::key_type>;

What do you think?

Comment on lines +176 to +182
template <class Element>
Element find_path(Element x)
{
splay(x);
return x;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if this function needs to exist?

Comment on lines +221 to +223
template <class ID = identity_property_map,
class InverseID = boost::unordered_map<typename property_traits<ID>::value_type, typename property_traits<ID>::key_type>,
class IndexMapContainer = boost::unordered_map<typename property_traits<ID>::value_type, typename property_traits<ID>::value_type> >
Copy link
Contributor

Choose a reason for hiding this comment

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

Boost.Unordered has a new, much faster, hash map called unordered_flat_map: https://www.boost.org/doc/libs/develop/libs/unordered/doc/html/unordered.html

It's not compliant with unordered_map, but I presume we don't need the extra features of the standard interface and can drop it in here.

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 this pull request may close these issues.

3 participants