Skip to content

Conversation

@angriman
Copy link
Member

@angriman angriman commented Apr 7, 2021

This PR brings the 1/2-approximation algorithm for maximum (weighted) matching by Manne and Halappanavar (New Effective Multithreaded Matching Algorithms, IPDPS 2014).
There are two versions of the algorithm, one of which only supports graphs where the adjacency lists of every vertex are sorted by edge weight in decreasing order. To this end I also added a new sortEdgesByWeight method in Graph, which allows to sort the edges of a graph by edge weight in increasing or decreasing order. This method uses a new generic sortEdges method in Graph, which allows to sort the edges of a graph according to a custom criterion.

I am a bit hesitant to add the new sorting methods to Graph, they are probably too specific. An alternative idea is to implement them in GraphTools.

@avdgrinten
Copy link
Contributor

Does the increase in runtime performance really justify the templated implementation? Can't you just branch on a sortSuitor variable at runtime?

Also, the sortEdgesByWeight method is not appropriate for Graph, as it can be implemented based on simpler primitives (i.e., the sortEdges method).

@angriman angriman force-pushed the suitor-matcher branch 2 times, most recently from a0d0702 to a2ab952 Compare April 9, 2021 13:44
@angriman
Copy link
Member Author

angriman commented Apr 9, 2021

Does the increase in runtime performance really justify the templated implementation? Can't you just branch on a sortSuitor variable at runtime?

I removed the templated implementation, it that was not really necessary.

Also, the sortEdgesByWeight method is not appropriate for Graph, as it can be implemented based on simpler primitives (i.e., the sortEdges method).

I moved it in GraphTools and it uses the sortEdges primitive.

michitux
michitux previously approved these changes Apr 23, 2021
Copy link
Contributor

@michitux michitux left a comment

Choose a reason for hiding this comment

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

In general this looks good now, I have just a few small comments that you may implement or not.

Do you have any idea how fast sorting neighbors by id with your new function is compared to the existing sortEdges functionality? My suspicion would be that due to increased locality it may be faster in practice, in particular when multiple cores are used. If it should be faster, I would suggest to replace the existing sortEdges method by your new method.

* @return @a edge weight to the i-th (outgoing) neighbor of @a u, or @c +inf if no such
* neighbor exists.
*/
edgeweight getIthNeighborWeight(node u, index i) const {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it intentional that this method is not added to the Python interface?

Copy link
Member Author

Choose a reason for hiding this comment

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

getIthNeighbor does not have a Python interface, so we either provide a Python interface for both or for none.

const auto sortAdjacencyArrays = [&](node u, std::vector<node> &adjList,
std::vector<edgeweight> &weights,
std::vector<edgeid> &edgeIds) -> void {
std::vector<index> indices(adjList.size());
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be faster to have one indices vector per thread that is only re-allocated when a node with a larger degree is encountered? The design with the new indices vector for every call is definitely clearer, what I'm wondering though is what impact this memory allocation has on performance.

Copy link
Member

Choose a reason for hiding this comment

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

Although the code slightly changed in the most recent version, I think time can still be safed if resize is only done if adjList.size() > indices.size(). Reasoning: resize calls erase on indices if adjList.size() < indices.size(), while iota is called anyways afterwards.

Copy link
Member Author

Choose a reason for hiding this comment

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

Now resize is called only when necessary.

@angriman angriman force-pushed the suitor-matcher branch 3 times, most recently from fb93872 to 5e36a9e Compare May 3, 2021 07:33
@angriman angriman mentioned this pull request May 25, 2021
@angriman angriman force-pushed the suitor-matcher branch 3 times, most recently from aa7dacf to ce447c7 Compare June 3, 2021 09:28
@angriman angriman force-pushed the suitor-matcher branch 2 times, most recently from 7900784 to 6f915a7 Compare June 24, 2021 08:38
fabratu
fabratu previously approved these changes Jun 24, 2021
Copy link
Member

@fabratu fabratu left a comment

Choose a reason for hiding this comment

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

LGTM.

@avdgrinten avdgrinten merged commit d0b3efa into networkit:master Jun 29, 2021
@avdgrinten avdgrinten deleted the suitor-matcher branch June 29, 2021 13:21
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.

5 participants