sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.37k stars 465 forks source link

Matchings in Bipartite Graphs #22559

Closed c703ac91-1c31-4f3a-abb2-47af3ac2705d closed 7 years ago

c703ac91-1c31-4f3a-abb2-47af3ac2705d commented 7 years ago

The Hopcroft-Karp and Eppstein algorithms as implemented in NetworkX compute matchings for bipartite graphs faster than the algorithms for general graphs. I've included an override of the matching method in the bipartite graph class, with parameters that allow for using the old algorithms if desired.

Component: graph theory

Author: Zach Gershkoff

Branch: 5924a34

Reviewer: Julian Rüth, Travis Scrimshaw, David Coudert

Issue created by migration from https://trac.sagemath.org/ticket/22559

c703ac91-1c31-4f3a-abb2-47af3ac2705d commented 7 years ago

Branch: u/zgershkoff/matchings_in_bipartite_graphs

c703ac91-1c31-4f3a-abb2-47af3ac2705d commented 7 years ago

Commit: 7f07803

c703ac91-1c31-4f3a-abb2-47af3ac2705d commented 7 years ago

New commits:

7f07803bipartite graph mathchings
c703ac91-1c31-4f3a-abb2-47af3ac2705d commented 7 years ago

Description changed:

--- 
+++ 
@@ -1 +1 @@
-
+The Hopcroft-Karp and Eppstein algorithms as implemented in NetworkX compute matchings for bipartite graphs faster than the algorithms for general graphs. I've included an override of the matching method in the bipartite graph class, with parameters that allow for using the old algorithms if desired.
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

22ba202libraries for compatibility
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

Changed commit from 7f07803 to 22ba202

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

d6215f1removed tab character
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

Changed commit from 22ba202 to d6215f1

saraedum commented 7 years ago

Work Issues: author is missing

saraedum commented 7 years ago
comment:5

zgershkoff, I could not find your handle on the wiki home page.

c703ac91-1c31-4f3a-abb2-47af3ac2705d commented 7 years ago

Author: Zachary Gershkoff

c703ac91-1c31-4f3a-abb2-47af3ac2705d commented 7 years ago
comment:6

I don't see anything in the developer's guide about this. Is this sufficient?

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

0da102afixed whitespace issue
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

Changed commit from d6215f1 to 0da102a

saraedum commented 7 years ago

Changed work issues from author is missing to none

saraedum commented 7 years ago
comment:9

Replying to @sagetrac-zgershkoff:

I don't see anything in the developer's guide about this. Is this sufficient?

Yes. It is probably not in the developer's guide. But the merge commit uses that information (and the Reviewer field) afaik. It would be great if you could also add yourself to the list on the wiki's homepage.

saraedum commented 7 years ago
comment:10

I made some minor changes. Let's see what the patchbots think about this.

saraedum commented 7 years ago

Changed branch from u/zgershkoff/matchings_in_bipartite_graphs to u/saraedum/matchings_in_bipartite_graphs

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

7f3cc61Merge branch 'develop' into t/22559/matchings_in_bipartite_graphs
32eb46dRevert "Import iteritems directly"
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

Changed commit from 0da102a to 32eb46d

saraedum commented 7 years ago

Reviewer: Julian Rüth

c703ac91-1c31-4f3a-abb2-47af3ac2705d commented 7 years ago
comment:14

The patchbot says it fails python3 compatibility when it calls six.iteritems, which I find confusing-- I thought the purpose of the six library was python2-3 compatibility.

90e8124e-743e-4424-88b0-22f5ae41372b commented 7 years ago
comment:15

Just a hunch here: maybe you need to write "for (u, v) in six.iteritems..."?

I couldn't find anything online justifying this.

fchapoton commented 7 years ago
comment:16

Patchbot is not smart enough to recognize the six before iteritems.

You should rather use

from six import iteritems

...

for u,v in iteritems
tscrim commented 7 years ago
comment:17

Three other minor things:

c703ac91-1c31-4f3a-abb2-47af3ac2705d commented 7 years ago

Changed branch from u/saraedum/matchings_in_bipartite_graphs to u/zgershkoff/matchings_in_bipartite_graphs

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

4cabf0bwrong kind of apostrophe
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

Changed commit from 32eb46d to 4cabf0b

c703ac91-1c31-4f3a-abb2-47af3ac2705d commented 7 years ago
comment:20

I made all the documentation changes tscrim suggested except the last one, which I believe saraedum already addressed.

Replying to @fchapoton:

Patchbot is not smart enough to recognize the six before iteritems.

I changed it to just iteritems in bipartite_graph.py, but the way I had it before is exactly how it was used in graph.py. I didn't fix it in graph.py since it's not broken.

tscrim commented 7 years ago
comment:21

No, Julian did not address it already; in particular there are still 2 commas separated by a space. I also (slightly) think you should aim to make it not use periods to be consistent and concise.

tscrim commented 7 years ago

Changed reviewer from Julian Rüth to Julian Rüth, Travis Scrimshaw

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

6dd7eaatypo and style fixes
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

Changed commit from 4cabf0b to 6dd7eaa

c703ac91-1c31-4f3a-abb2-47af3ac2705d commented 7 years ago
comment:23

Ok, I see. Thank you for the feedback. I fixed the things you pointed out, but I changed the phrasing away from "ignored if algorithm is not "Edmonds", "LP"" because it's now configured to raise an error if use_edge_labels==True but algorithm is not set to "Edmonds" or "LP".


New commits:

6dd7eaatypo and style fixes
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

Changed commit from 6dd7eaa to a20b717

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

a20b717typo fix
tscrim commented 7 years ago
comment:25

You need to revert back the indentation by 2 spaces.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

Changed commit from a20b717 to e3c353e

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

e3c353efixed indentation
tscrim commented 7 years ago
comment:27

You also need to do the other two lines. See comment:17 (also, it would be good if the arguments for the function started at the same level, it's a PEP8 thing).

c703ac91-1c31-4f3a-abb2-47af3ac2705d commented 7 years ago
comment:28

Replying to @tscrim:

You also need to do the other two lines. See comment:17 (also, it would be good if the arguments for the function started at the same level, it's a PEP8 thing).

I believe the lines you're referring to are addressed in this commit https://github.com/sagemath/sagetrac-mirror/commit/5339528b85dd389a3e7633678fe609796fe787b0 - I'm not sure why it didn't show up in the trac ticket.

I'll look into PEP8 and make changes shortly.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

Changed commit from e3c353e to fadefcb

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

fadefcbPEP8 compliance
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

Changed commit from fadefcb to 96b1242

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

96b1242Removed tab character from new lines
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

Changed commit from 96b1242 to 0d0bbbc

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

0d0bbbcnewline at end of file
90e8124e-743e-4424-88b0-22f5ae41372b commented 7 years ago
comment:32

I think this looks good now. Travis, do you agree?

tscrim commented 7 years ago
comment:33

Whoops, sorry this fell of my radar. Yep, looks good.

dcoudert commented 7 years ago
comment:34

Why don't you use G.networkx_graph() to get the networkx version of the graph ?