Skip to content
This repository has been archived by the owner on Oct 8, 2021. It is now read-only.

matching algorithm for bipartite graphs (linear programming) #207

Merged
merged 32 commits into from
Nov 6, 2015

Conversation

CarloLucibello
Copy link
Contributor

partially fixes #194

This is meant to be a temporary solution, exploiting the fact that for bipartite graphs matching can be relaxed to linear programming, untill someone finds the courage and the will to implement a more effcient matching algorithm.

For the time being I added JuMP to REQUIRE, I don't know how to set them as optional dependences.

@CarloLucibello
Copy link
Contributor Author

looks like there is a problem on Julia 0.5

end

"""
As `maximum_weigth_maximal_matching`, with the difference that the edges `e` with
Copy link
Contributor

Choose a reason for hiding this comment

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

weight not weigth

@sbromberger
Copy link
Owner

I don't like the idea of adding complex dependencies like JuMP for a single (set of) functions. If we can't eliminate the dependency, it looks like it might be better as a separate package.

@jpfairbanks
Copy link
Contributor

@sbromberger, there are lots of graph problems that can be solved with a general purpose optimization tool like jump. Once we have easy access to such a tool it will be easier to add new functionality that leverages it.

Can we make it a Conditional dependency?

It plays a similar role to spectral methods, once you have a good linear solver on Graphs there are lots of ways to use it for analyzing graphs.

@sbromberger
Copy link
Owner

Conditional dependency is fine, but we should make sure that we've got more than one function that uses it.

@CarloLucibello
Copy link
Contributor Author

Why we need more than one function? Having an algorithm for matching should be reason enough. IHaving an algorithm for matching is a worthwhile addition, it is an important problem in combinatorial optimization, it has a lot of applications, and on top of that few libreries iimplemnt it, so it adds value to LightGraphs

@sbromberger
Copy link
Owner

It just seems to me that requiring JuMP - which itself requires 4 different packages that are not part of LightGraphs REQUIRE (which themselves require 4+ packages that aren't part of LG REQUIRE) - just for a single function seems like it's too much extra baggage.

@CarloLucibello
Copy link
Contributor Author

But conditional dependence means it should not go into REQUIRE, right?

@sbromberger
Copy link
Owner

That's right, and why I think we're ok with conditional dependence. But I think we still need to ensure we've got more than one function that will use it. This is a lot of baggage for a single enhancement.

@CarloLucibello
Copy link
Contributor Author

So, should I modify the PR to make it conditional? and how?

@sbromberger
Copy link
Owner

I'd like @jpfairbanks to weigh in here – will this be useful for other functionality or is a JuMP conditional just good for matching?

@codecov-io
Copy link

Current coverage is 100.00%

Merging #207 into master will not affect coverage as of 0249853

Powered by Codecov. Updated on successful CI builds.

@CarloLucibello
Copy link
Contributor Author

If I don't add JuMP to REQUIRE, how do I say to Travis to add it for testing?

@jpfairbanks
Copy link
Contributor

I think that matching is important and that using jump will support more functions in the future. You can have travis Pkg.add jump and the call the tests just like we do for graph matrices.

Once you have a powerful tool like jump, you find ways to use it. :)

status != :Optimal && error("Failure")
sol = getValue(x)

all(Bool[s == 1 || s == 0 for s in sol])
Copy link
Owner

Choose a reason for hiding this comment

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

What is this statement supposed to do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since this is a continuous relaxation of an integer problem, here I check that the solution takes only integer values

Copy link
Owner

Choose a reason for hiding this comment

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

But the statement doesn't do anything. It evaluates to true or false and then discards the answer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, sure, that was meant to be in an assert. I'll fix it

@CarloLucibello
Copy link
Contributor Author

this is ready for merge


#matching
maximum_weight_maximal_matching,

Copy link
Owner

Choose a reason for hiding this comment

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

Is this a duplicate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes

@CarloLucibello
Copy link
Contributor Author

I renamed pi and \pi to m. I didn't remove the Int[] constructors, since I like them, they are much more concise then Vector{Int}(), and moreover they are used all over the place, e.g.

"""Add a new vertex to the graph `g`."""
function add_vertex!(g::SimpleGraph)
    n = length(vertices(g)) + 1
    g.vertices = 1:n
    push!(g.badjlist, Int[])
    push!(g.fadjlist, Int[])

    return n
end

@sbromberger
Copy link
Owner

I've been weeding them out; please file issues if you see Int[] constructors. See also https://groups.google.com/d/msg/julia-users/BPPjnk20JgY/Ihq6KMqkS-UJ.

@sbromberger
Copy link
Owner

@jpfairbanks - last call before merge. All good?

@jpfairbanks
Copy link
Contributor

Go for it.
On Nov 5, 2015 4:42 PM, "Seth Bromberger" notifications@github.com wrote:

@jpfairbanks https://github.com/jpfairbanks - last call before merge.
All good?


Reply to this email directly or view it on GitHub
#207 (comment)
.

sbromberger added a commit that referenced this pull request Nov 6, 2015
matching algorithm for bipartite graphs (linear programming)
@sbromberger sbromberger merged commit 492523f into sbromberger:master Nov 6, 2015
@sbromberger
Copy link
Owner

All done. Thanks, @CarloLucibello and @jpfairbanks !!

@CarloLucibello CarloLucibello deleted the matching2 branch November 7, 2015 15:48
@sbromberger
Copy link
Owner

I really think we need to revert this merge. It's causing travis to take > 10 mins to build all the JuMP dependencies, and we're seeing errors due to the macros and conditional dependencies.

I'm in favor of revisiting once we understand the impact, but requiring JuMP in LightGraphs is making it too top-heavy right now.

@CarloLucibello
Copy link
Contributor Author

I'd say let's exclude matching just from testing, so that we don't also have to load JuMP. If this cannot be done easily, or if it would be bad practice to leave out from testing, let's just revert this. Also I don't know if it's possible to have codecov ignore the matching part, I wouldn't like it always complaining

@jpfairbanks
Copy link
Contributor

Can we find a way to make travis faster with JuMP? What about storing all the dependencies prebuilt for travis? The JuMP repo uses this travis.yml file which installs glpk from the apt repo on linux.
This technique should reduce the travis build time.

language: julia
os:
    - linux
    - osx
julia:
    - 0.4
    - nightly
notifications:
    email: false
sudo: false
addons:
    apt_packages:
        - gfortran
        - liblapack-dev
        - libgmp-dev
        - libglpk-dev
script:
    - if [[ -a .git/shallow ]]; then git fetch --unshallow; fi
    - julia -e 'Pkg.clone(pwd())'
    - julia -e 'Pkg.test("JuMP", coverage=true)'
    - julia test/hygiene.jl
    - julia -e 'Pkg.add("Ipopt")' # needed for below tests
    - julia test/hockschittkowski/runhs.jl
after_success:
    - echo $TRAVIS_JULIA_VERSION
    - julia -e 'Pkg.add("Coverage"); cd(Pkg.dir("JuMP")); using Coverage; Coveralls.submit(process_folder()); Codecov.submit(process_folder())'

For more info: http://docs.travis-ci.com/user/installing-dependencies/

@sbromberger
Copy link
Owner

See https://groups.google.com/d/msg/julia-users/L1_E5uuSj0M/rdtJ_3mdBQAJ for a possible solution to the dependency issue.

@jpfairbanks
Copy link
Contributor

yup that looks to be the right thing to allow LightGraphs to run without a dependence on JuMP. We should also use apt-get to test with JuMP on linux.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

matching algorithm
4 participants