jpfairbanks / GraphMatrices.jl

A Julia package for strongly typed graph matrices.
Other
6 stars 5 forks source link

Light graphs integration #4

Closed jpfairbanks closed 9 years ago

jpfairbanks commented 9 years ago

This PR addresses #1. This builds when LightGraphs is installed. We need to find a home for this code:

function CombinatorialAdjacency(A)
    D = indegree(A, vertices(A))
    return CombinatorialAdjacency{Float64, typeof(A), typeof(D)}(A,D)
end

and the tests.

jpfairbanks commented 9 years ago

This is a tiny change and a huge diff because I refactored the tests into functions.

sbromberger commented 9 years ago

It looks like the only LightGraphs-specific code is in tests. If that's the case, perhaps you don't need LightGraphs in the REQUIRE?

Or is it the above function that requires it (or requires LightGraphs to use GraphMatrices)? If so, we could do something like

_HAVE_LIGHTGRAPHS = try
        using LightGraphs
        true
    catch
        false
    end

...

if _HAVE_LIGHTGRAPHS
        function CombinatorialAdjacency(G::SimpleGraph)
...
end
jpfairbanks commented 9 years ago

It makes sense to me that the GraphMatrices code be storage format independent and that any type that supports the necessary operations can just automagically work. It took an afternoon for me to get it working with LightGraphs as the storage format.

The question is whether GraphMatrices should test that LightGraphs is a working backend or if LightGraphs should test that it is compatible with GraphMatrices? That will answer the question of who should depend on whom.

Since LightGraphs will be changing more often (it is a larger project) it makes sense for LightGraphs to test compatibility. Thus I can put these tests into LightGraphs and LightGraphs can REQUIRE GraphMatrices. Then anyone using LightGraphs will have an easy way to get the benefits of GraphMatrices.

sbromberger commented 9 years ago

We can also make it a conditional within LightGraphs - that is, if GraphMatrices is available, we can then define these functions. If it's not, then no need.

jpfairbanks commented 9 years ago

This was handled by #6