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

Maximum weighted matching #23

Merged
merged 3 commits into from
Mar 12, 2017
Merged

Conversation

mburq
Copy link
Contributor

@mburq mburq commented Feb 16, 2017

Hi,

This is a formulation for maximum weighted matching, works for both bipartite and non-bipartite graphs.
In the case of bipartite graphs, the solution of the LP is automatically integral (i.e. no need to enforce integrality constraints).
In the case of non-bipartite graphs, it requires an MIP solver (e.g. GLPK) and may therefore be much slower to solve to optimality.

Disclaimer: This is the first time I do a pull request, so I hope I'm not missing an important step.

@codecov-io
Copy link

codecov-io commented Feb 16, 2017

Codecov Report

Merging #23 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master    #23   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          13     14    +1     
  Lines         210    234   +24     
=====================================
+ Hits          210    234   +24
Impacted Files Coverage Δ
src/matching/matching.jl 100% <ø> (ø)
src/matching/maximum_weight_matching.jl 100% <100%> (ø)
src/interdiction/bilevel_adaptive_arc.jl 100% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 63b870c...5687734. Read the comment docs.

@@ -1,7 +1,7 @@
__precompile__(true)
module Matching
using LightGraphs
export MatchingResult, maximum_weight_maximal_matching, minimum_weight_perfect_matching
export MatchingResult, max_weight_matching, maximum_weight_maximal_matching, minimum_weight_perfect_matching
Copy link
Member

Choose a reason for hiding this comment

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

max_weight_matching->maximum_weight_matching

for coerence with the other functions


Given a graph `g` and an edgemap `w` containing weights associated to edges,
returns a matching with the maximum total weight.
`w` is a dictionnary that maps edges i => j (with i <= j) to weights.
Copy link
Member

Choose a reason for hiding this comment

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

typo here

@CarloLucibello
Copy link
Member

CarloLucibello commented Feb 17, 2017

"- If the graph is not bipartite, then the computation time may grow exponentially."

I do not see integer programming part in case of fractionary solution, why do you say that?

I see three options here:

  • @Assert is_bipartite(g)
  • accept also bipartite graphs, but caution the user about the possible suboptimality of the solution, possibly returning also a fractional_solution=true flag
  • add an Integer programming part in case of fractional solutions

@CarloLucibello
Copy link
Member

also, coverage should be increased

@mburq
Copy link
Contributor Author

mburq commented Feb 17, 2017

Sorry I wasn't clear in my comments, the integer programming is taken care of by the solver (by using the Int keyword in @variable, line 45).
Good idea to use is_bipartite(g) so that it doesn't require a an Integer Programming solver if the graph is bipartite.


Given a graph `g` and an edgemap `w` containing weights associated to edges,
returns a matching with the maximum total weight.
`w` is a dictionnary that maps edges i => j to weights.
Copy link
Member

Choose a reason for hiding this comment

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

dictionnary->dictionary

The package JuMP.jl and one of its supported solvers is required.

Returns MatchingResult containing:
- a solve status (indicating whether the problem was solved to optimality)
Copy link
Member

Choose a reason for hiding this comment

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

MatchingResult doesn't contain solve

Copy link
Member

Choose a reason for hiding this comment

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

ok, I saw the TODO comment

Returns MatchingResult containing:
- a solve status (indicating whether the problem was solved to optimality)
- the optimal cost
- a list of each node's match.
Copy link
Member

Choose a reason for hiding this comment

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

-> a list of each vertex's match (or -1 for unmatched vertices)

w[Edge(1,3)] = 1
w[Edge(1,4)] = 3
w[Edge(2,4)] = 1
match = Matching.maximum_weight_matching(g,w)
Copy link
Member

Choose a reason for hiding this comment

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

can you remove Matching. from here and from the other lines in this file (also a check to see that everything is correctly exported)

@CarloLucibello
Copy link
Member

Sorry I wasn't clear in my comments, the integer programming is taken care of by the solver (by using the Int keyword in @variable, line 45).
Good idea to use is_bipartite(g) so that it doesn't require a an Integer Programming solver if the graph is bipartite.

Ok, thanks for the clarification, my knowledge of JuMP is very limited. This looks good, i can be merged once the remaining comments are addressed

@CarloLucibello
Copy link
Member

this case should be tested

if setdiff(edge_list, keys(w)) != [] # If some edges do not have a key in w.
      error("Some edge weights are missing, check that keys i => j in w satisfy i <= j")
end

with a

@test_throws ErrorException ....

@mburq
Copy link
Contributor Author

mburq commented Mar 12, 2017

Just a quick follow-up: can this PR be merged?
Thanks @CarloLucibello for the help in the review process.

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.

4 participants