Add max weight matching function #229

mtreinish · 2021-01-17T18:11:22Z

This commit adds a new python function max_weight_matching() for
computing the maximum-weighted matching of a PyGraph object. The
implementation of this function is based on “Efficient Algorithms for
Finding Maximum Matching in Graphs”, Zvi Galil, ACM Computing Surveys,
1986. [1] It is basically a porting of the networkx implementation of
the algorithm [2] with some additional inspiration from the prototype for
the networkx version (it's basically the same code but some aspects were
a bit clearer to figure out from the prototype rather than networkx's
copy of it). [3][4]

Fixes #216

[1] https://dl.acm.org/doi/10.1145/6462.6502
[2] https://github.com/networkx/networkx/blob/3351206a3ce5b3a39bb2fc451e93ef545b96c95b/networkx/algorithms/matching.py
[3] http://jorisvr.nl/article/maximum-matching
[4] http://jorisvr.nl/files/graphmatching/20130407/mwmatching.py

TODO:

Fix test failures
Change output from function to be set of tuples for each matching pair instead of dict?

This commit adds a new python function max_weight_function() for computing the maximum-weighted matching of a PyGraph object. The implementation of this function is based on “Efficient Algorithms for Finding Maximum Matching in Graphs”, Zvi Galil, ACM Computing Surveys, 1986. [1] It is basically a porting of the networkx implementation of the algorithm [2] with some additional inspiration from the prototype for the networkx version (it's basically the same code but some aspects were a bit clearer to figure out from the prototype rather than networkx's copy of it). [3][4] Fixes Qiskit#216 [1] https://dl.acm.org/doi/10.1145/6462.6502 [2] https://github.com/networkx/networkx/blob/3351206a3ce5b3a39bb2fc451e93ef545b96c95b/networkx/algorithms/matching.py [3] http://jorisvr.nl/article/maximum-matching [4] http://jorisvr.nl/files/graphmatching/20130407/mwmatching.py

This commit fixes all but 2 of the test panics. There were 2 classes of issues that his commit fixes (both were artifacts of sloppy porting from Python). The first is that in add_blossom() we were not updating the global blossom_children or the global blossom_endpoints while iterating over the list. Instead we were just setting a local. The second issue was negative indices for Vecs. The python code was iterating over elements in a vec in expand_blossom() and augment_blossom() by subtracting a length or step from the index which could result in a negative value. In Python this is fine since a negative index is just the from the last element. But, for rust we need to handle this behavior manually because the Index trait only deals with usize.

coveralls · 2021-01-18T20:46:13Z

Pull Request Test Coverage Report for Build 593928485

1133 of 1145 (98.95%) changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.6%) to 96.412%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/max_weight_matching.rs	1110	1122	98.93%

Totals
Change from base Build 593927154:	0.6%
Covered Lines:	5697
Relevant Lines:	5909

💛 - Coveralls

mtreinish · 2021-01-18T21:30:50Z

This is ready to review now. The only open question is on the return format. Right now it is returning a dict but networkx returns a set of the matching nodes. It's not hard to adjust it, but I'm not sure what people think is a better choice.

This commit fixes an issue when running max_weight_matching on a graph with holes in the node indices (i.e. when 'node_count() != max(node_indices)'). The in_blossoms vec was being initialized from the contents of the node_indices iterator of the graph but the algorithm remaps the indices to be a contiguous list for the internals to work. When a hole was encountered accessing in_blossoms would panic because the index returned wouldn't match the expecated state. This is fixed and a test added to make sure we have coverage on input graphs with holes.

t-imamichi · 2021-01-25T08:56:13Z

It might be good to add a unit test to compare results of networkx and retworkx with random graphs.

mtreinish · 2021-01-25T12:59:35Z

It might be good to add a unit test to compare results of networkx and retworkx with random graphs.

Sure, I added a couple of tests to do that in: 308c82a

itoko

Thank you for this great job. I've just started to read but not yet completed. I have one question so far. Is there any reason to use endpoints data structure instead of vertex pairs? (Is it just for performance?)

src/max_weight_matching.rs

mtreinish · 2021-02-22T15:06:51Z

Ok, sorry for the delay I've added a fallback check for the cases where networkx and retworkx return differing matchings to verify the retworkx matching is still correct (I hope I implemented it correctly). I also added a test that assigns a random integer weight to each edge.

mtreinish · 2021-02-22T15:34:56Z

As an aside, these tests are the slowest in the suite (taking 3-4 seconds) so I was curious where the bottleneck was and ran a profile of the test class. The slowdown is from running networkx's max_weight_matching(), but it actually gives us a good performance comparison against the retworkx version of the function being added here:

library	cumulative time (sec)	number of calls	time per call (sec)
retworkx	0.1376	5150	2.672e-05
networkx	7.146	5123	0.001395

These numbers are over the entire test class including the test methods which do not call networkx (also they have cProfile overhead which will be a bit worse for networkx because it has a deeper python call stack).

t-imamichi · 2021-02-24T09:28:54Z

tests/test_max_weight_matching.py

+        len(set(e1) & set(e2)) == 0 for e1, e2 in combinations(matching, 2))
+
+
+def is_maximal_matching(graph, rx_matches):


is_matching and is_maximal_matching might be useful if they are included as part of retworkx. What do you think?

Yeah I agree, it'll be good thing to add as a retworkx function in a follow up PR (this one is already big enough :) ). I've opened an issue on this here: #255.

This commit adds new functions for checking a provided matching set is valid, is_matching(), and that a provided matching set is valid and maximal, is_maximal_set(). This pairs with Qiskit#229 and can be used to partially check the output from the max_weight_matching function added there. Equivalent functions were implemented in Qiskit#229 using Python for the tests in Qiskit#229 and those tests should be updated to use these functions instead. Fixes Qiskit#255

itoko

Thank you for updating the tests! LGTM

t-imamichi

LGTM. Great job!

* Add is_maximal_matching and is_matching functions This commit adds new functions for checking a provided matching set is valid, is_matching(), and that a provided matching set is valid and maximal, is_maximal_set(). This pairs with #229 and can be used to partially check the output from the max_weight_matching function added there. Equivalent functions were implemented in #229 using Python for the tests in #229 and those tests should be updated to use these functions instead. Fixes #255 * Use retworkx functions in max_weight_matching tests * Remove unused import * Simplify is_matching logic The original implementation was based on networkx's which was getting all the combinations of edges in the matching and checking for unique endpoints on all those pairs. However, doing the combinations adds extra complexity when we can just flatten the input matching to a set of endpoints and if aduplicate is found it's not a valid matching. This commit updates the inner is_matching function to make this change which simplifies the code and should be faster. Co-authored-by: Toshinari Itoko <itoko@jp.ibm.com> * Update docstring Co-authored-by: Toshinari Itoko <itoko@jp.ibm.com>

Since Qiskit#229 our test runs take significantly longer. This is mostly a function of the tests added in that function running over numerous random graphs and comparing the output against networkx to verify it works in all cases. However, this validation comes with a runtime cost. In an attempt to mitigate this partially this commit changes the test runner from stdlib's unittest runner to use stestr. [1] stestr provide a performance improvement by running tests on parallel workers and also provides an easier to use UI that enables test selection a history of previous runs, and many other features. The test suite in retworkx will still need to be strictly unittest compatible so anyone can run with stestr, stdlib unittest, pytest, or any other test runner of their choice. But the default in tox and CI will be to use stestr for the performance.

Since Qiskit#229 our test runs take significantly longer. This is mostly a function of the tests added in that function running over numerous random graphs and comparing the output against networkx to verify it works in all cases. However, this validation comes with a runtime cost. In an attempt to mitigate this partially this commit changes the test runner from stdlib's unittest runner to use stestr. [1][2][3] stestr provides a performance improvement by running tests on parallel workers and also provides an easier to use UI that enables test selection a history of previous runs, and many other features. The test suite in retworkx will still need to be strictly unittest compatible so anyone can run with stestr, stdlib unittest, pytest, or any other test runner of their choice. But the default in tox and CI will be to use stestr for the performance benefit. [1] https://pypi.org/project/stestr/ [2] https://github.com/mtreinish/stestr [3] https://stestr.readthedocs.io/en/stable/README.html

Since #229 our test runs take significantly longer. This is mostly a function of the tests added in that function running over numerous random graphs and comparing the output against networkx to verify it works in all cases. However, this validation comes with a runtime cost. In an attempt to mitigate this partially this commit changes the test runner from stdlib's unittest runner to use stestr. [1][2][3] stestr provides a performance improvement by running tests on parallel workers and also provides an easier to use UI that enables test selection a history of previous runs, and many other features. The test suite in retworkx will still need to be strictly unittest compatible so anyone can run with stestr, stdlib unittest, pytest, or any other test runner of their choice. But the default in tox and CI will be to use stestr for the performance benefit. [1] https://pypi.org/project/stestr/ [2] https://github.com/mtreinish/stestr [3] https://stestr.readthedocs.io/en/stable/README.html

mtreinish requested review from itoko and t-imamichi January 17, 2021 18:11

mtreinish added this to the 0.8.0 milestone Jan 17, 2021

mtreinish added the on hold label Jan 17, 2021

mtreinish force-pushed the max_weight_matching branch 2 times, most recently from 953f364 to 481f99a Compare January 17, 2021 18:14

mtreinish force-pushed the max_weight_matching branch from 481f99a to 98fa26e Compare January 17, 2021 18:14

mtreinish added 5 commits January 17, 2021 16:50

Fix panic in one test

4f54cdc

Remove unused verify_optimum() function

b25f9bf

Remove unnecessary BlossomList struct and just use Vec<Vec<usize>>

ecf1f09

Fix tests

85b96e1

mtreinish changed the title ~~[WIP] Add max weight matching function~~ Add max weight matching function Jan 18, 2021

mtreinish removed the on hold label Jan 18, 2021

Fix docs build

48f3192

mtreinish added 6 commits January 20, 2021 07:50

Fix typos in code comments

b7ee890

Merge branch 'master' into max_weight_matching

83776c7

Merge branch 'master' into max_weight_matching

91c7f87

Merge branch 'master' into max_weight_matching

9bbffa9

Merge branch 'master' into max_weight_matching

7a72299

Add test to compare output with random graph to networkx

308c82a

mtreinish added 2 commits January 25, 2021 08:06

Use tox for non-x86 travis ci jobs

6239be7

Add networkx to coverage job

4d48492

itoko reviewed Jan 26, 2021

View reviewed changes

src/max_weight_matching.rs Outdated Show resolved Hide resolved

src/max_weight_matching.rs Show resolved Hide resolved

mtreinish added 6 commits February 21, 2021 09:14

Merge branch 'master' into max_weight_matching

18c13f0

Use HashMap for mate instead of Vec

dc5d215

Switch best_edge_to to HashMap

71780e1

Merge branch 'master' into max_weight_matching

a2fdbb4

Add test with weights

1b59c97

Fix nx miss check

6dc8ea5

mtreinish added 3 commits February 22, 2021 10:10

Add max cardinality random weight test

2a7989b

Add random test cases with negative weights too

a117f75

Use subtest for each iteration of random loop

0418c50

Add missing tox install to travis config

973267e

mtreinish requested review from itoko and t-imamichi February 23, 2021 18:19

Merge branch 'master' into max_weight_matching

8570e9a

t-imamichi reviewed Feb 24, 2021

View reviewed changes

mtreinish mentioned this pull request Feb 24, 2021

Add is_matching and is_maximal_matching functions #255

Closed

mtreinish mentioned this pull request Feb 24, 2021

Add is_maximal_matching and is_matching functions #256

Merged

itoko approved these changes Feb 25, 2021

View reviewed changes

t-imamichi approved these changes Feb 25, 2021

View reviewed changes

mtreinish merged commit 7e889b2 into Qiskit:master Feb 25, 2021

mtreinish deleted the max_weight_matching branch February 25, 2021 13:27

mtreinish mentioned this pull request Mar 8, 2021

Use stestr for test jobs #266

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add max weight matching function #229

Add max weight matching function #229

mtreinish commented Jan 17, 2021 •

edited

Loading

coveralls commented Jan 18, 2021 •

edited

Loading

mtreinish commented Jan 18, 2021

t-imamichi commented Jan 25, 2021

mtreinish commented Jan 25, 2021

itoko left a comment

mtreinish commented Feb 22, 2021

mtreinish commented Feb 22, 2021

t-imamichi Feb 24, 2021

mtreinish Feb 24, 2021

itoko left a comment

t-imamichi left a comment

		len(set(e1) & set(e2)) == 0 for e1, e2 in combinations(matching, 2))


		def is_maximal_matching(graph, rx_matches):

Add max weight matching function #229

Add max weight matching function #229

Conversation

mtreinish commented Jan 17, 2021 • edited Loading

coveralls commented Jan 18, 2021 • edited Loading

Pull Request Test Coverage Report for Build 593928485

💛 - Coveralls

mtreinish commented Jan 18, 2021

t-imamichi commented Jan 25, 2021

mtreinish commented Jan 25, 2021

itoko left a comment

Choose a reason for hiding this comment

mtreinish commented Feb 22, 2021

mtreinish commented Feb 22, 2021

t-imamichi Feb 24, 2021

Choose a reason for hiding this comment

mtreinish Feb 24, 2021

Choose a reason for hiding this comment

itoko left a comment

Choose a reason for hiding this comment

t-imamichi left a comment

Choose a reason for hiding this comment

mtreinish commented Jan 17, 2021 •

edited

Loading

coveralls commented Jan 18, 2021 •

edited

Loading