take advantage of short circuiting any()/all() in is_bipartite()

[sbromberger]

Ref: JuliaLang/julia#11774

[hayd]

The annoying part here is this creates a temporary array and is_bipartite is now always called for every vertex. :s (Edit: IIUC)

[sbromberger]

Ooh. yes. You're absolutely right. I think I can do this via

is_bipartite(g::SimpleGraph) = all(x->is_bipartite(g,x), vertices(g))

Do you agree?

[hayd]

That looks fine. But is there a performance penalty to using a lambda like this? (I don't quite get the recent discussions about functors/lambdas... or if that's even relevant.) If there is a performance penalty then I'm not sure this change is worth it!

[sbromberger]

I will test it and report back :)

[sbromberger]

Here's what we have:

    is_bipartite1(g::SimpleGraph) = all([is_bipartite(g,v) for v in vertices(g)])
    is_bipartite2(g::SimpleGraph) = all(x->is_bipartite(g,x), vertices(g))
    function is_bipartite(g::SimpleGraph)
        for v in vertices(g)
            if !is_bipartite(g, v)
                return false
            end
        end
        return true
    end

with results for a (10k, 100k) random digraph:

julia> @time LightGraphs.is_bipartite(g)
  31.225 microseconds (21 allocations: 94832 bytes)
false

julia> @time LightGraphs.is_bipartite1(g)
 388.163 milliseconds (170 k allocations: 910 MB, 29.60% gc time)
false

julia> @time LightGraphs.is_bipartite2(g)
  33.075 microseconds (25 allocations: 94944 bytes)
false

So it looks like is_bipartite2() is equivalent to the original, and much simpler. I'd rather rely on the Base-native short-circuiting behavior. Thoughts?

[sbromberger]

More timings on a 300k, 500k graph:

julia> @time LightGraphs.is_bipartite(g)
 570.103 microseconds (24 allocations: 2653 KB)
false

julia> @time LightGraphs.is_bipartite2(g)
 233.220 microseconds (28 allocations: 2654 KB)
false

[hayd]

One (weak) reason not to do this is coverage: we will no longer get a report of when this returns true. It looks cleaner to me, but on the other hand you have to keep the 0.3 version around... Up to you!

Edit: ok, second timing is convincing :) - don't understand that!

[sbromberger]

Let's defer this until 0.4 is released so that we can just deal with the performance regression in 0.3 by ignoring it :)