Replace hopcroftkarp with scipy.sparse.csgraph.maximum_bipartite_matching - #108
Replace hopcroftkarp with scipy.sparse.csgraph.maximum_bipartite_matching#108sushovan4 wants to merge 1 commit into
Conversation
…hing hopcroftkarp is GPLv3 and was last released 2019-10-11, which makes a permissive dependency closure impossible for persim and, transitively, for ripser. scipy provides the same algorithm, and persim already imports scipy in five modules -- including scipy.sparse.csgraph itself, in gromov_hausdorff.py -- so this removes a dependency rather than trading one for another. scipy was not declared, arriving transitively through scikit-learn. It is now declared explicitly, which is correct independently of this change. The two implementations differ in what they return. hopcroftkarp's maximum_matching() gives a dict carrying both directions, so a perfect matching was detected by its length being twice the row count and consumed by string key. maximum_bipartite_matching(perm_type="column") gives an array of column indices with -1 for unmatched, so the test becomes "no row is unmatched" -- equivalent because D is square -- and the lookup becomes integer indexing. Verified against the previous implementation on 300 random diagram pairs: bottleneck distances agree exactly in every case, and every returned matching realises the distance it reports under both implementations. The returned matching is not always identical: where several optimal matchings exist, the two algorithms break ties differently. No trial produced more than one edge at the bottleneck cost, so the differences fall entirely among the non-binding assignments. Two tests added: one pinning the matching on a pair whose optimum is unique, so tie-breaking cannot mask a regression, and one asserting the indices form a permutation, which is the property that would silently break if the array's orientation were misread.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #108 +/- ##
==========================================
- Coverage 79.40% 78.85% -0.56%
==========================================
Files 20 20
Lines 1481 1480 -1
Branches 271 270 -1
==========================================
- Hits 1176 1167 -9
- Misses 229 235 +6
- Partials 76 78 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Thanks for releasing the workflow run. Everything is green except Running the suite on
Codecov reports the opposite: 79.40% to 78.85%, −0.56%, on 1481 to 1480 lines. That implies roughly nine covered lines lost, which does not appear locally at all. Its base is current ( Happy to rebase onto Two things from the description that are the actual review surface, repeated here since the run has been sitting a while: The matching may differ from the old implementation where several optima exist. Across 300 random diagram pairs the distances agree exactly in all 300, and every returned matching realises the distance it reports under both implementations — but the matchings themselves differ in 137. No trial had more than one edge at the bottleneck cost, so the differences are entirely among non-binding assignments. Your existing The return shape changes, so three sites move, not one. No rush from my side. Glad to make any of the above changes if you would prefer them in before merge. |
1 participant
Closes #106.
Keeps to the two changes you named — the dependency and the code — plus tests.
The swap
hopcroftkarpis used in exactly two places, both inbottleneck.py.scipy.sparse.csgraph.maximum_bipartite_matchingis the same algorithm, and persim already imports scipy in five modules — includingscipy.sparse.csgraphitself ingromov_hausdorff.py— so this removes a dependency rather than trading one for another.scipy was not declared; it arrived transitively through scikit-learn. It is declared now, which is correct independently of this change.
The one thing that isn't a straight substitution
The two functions return different shapes:
hopcroftkarplen(res) == 2 * D.shape[0]res["{}".format(i)]scipy-1unmatchednot np.any(res == -1)res[i]The length test worked because the dict carries both sides, so
2nentries meant every row and every column was matched.Dis square, so "no row unmatched" is equivalent, and that is what the code now checks.np.infentries behave the same under both:inf <= dis false for finited, which is what excludes forbidden matches, and true atd = inf, so a perfect matching always exists at the largest threshold and the binary search stays well-founded.Verification
Beyond the suite (33 passing), I ran the new implementation against the released one on 300 random diagram pairs, sizes 1–5 on each side:
That last point is the one worth your judgement. Where several optimal matchings exist, the two algorithms break ties differently. No trial produced more than one edge at the bottleneck cost, so the differences fall entirely among the non-binding assignments: the max edge is the same, the pairs below it are a free choice, and both make a valid one.
So the distance is unchanged, and
matching=Truemay return a different — equally optimal — matching than before. Your existingtest_matchingchecks that every point appears rather than pinning indices, so nothing in the suite depended on the old choice, but a downstream user asserting on exact indices would notice. Happy to add a note to the docstring saying the matching is an optimal matching rather than the one, if you'd like that stated.Tests
Two added, both aimed at what a shape change could silently break:
test_matching_is_exact_on_an_unambiguous_pair— pins the actual pairs on a diagram pair whose optimum is unique (cross-matching costs 1.1, the diagonal costs at least 1.0, against 0.1 for the right answer), so tie-breaking cannot mask a regression.test_matching_indices_form_a_permutation— asserts each side is used exactly once. An off-by-one or a misread orientation would still produce well-formed output while pairing the wrong points, and nothing in the suite would have caught it.