Kamada kawai #89
Kamada kawai #89
Conversation
src/KamadaKawai.jl
Outdated
| @@ -0,0 +1,87 @@ | |||
| import Optim | |||
| using Distances | |||
| function kamada_kawai_layout(G, X=nothing; C= 1.0, MAXITER=100 ) | |||
There was a problem hiding this comment.
I think we need some kind of docstring here, explaining what happens.
Also we should use lower capitel letters for variable names.
There was a problem hiding this comment.
Not a problem. I've never done a non-trivial pull-request, so bear with me as I work my way through the process.
There was a problem hiding this comment.
I see that stressmajorize_layout using upper case variable names. I personally prefer CamelCase for Variables and camelCase for functions.
There was a problem hiding this comment.
You probably mean MAXITER more so than X or G. Done.
src/KamadaKawai.jl
Outdated
| 2.0*((z - a)/(b - a)) - 1.0 | ||
| end | ||
| # Stack individual graphs next to each other | ||
| for SubGraphVertices in connected_components(G) |
There was a problem hiding this comment.
Good call on that, I think that is something that we might have to tackle for graph layouts that we have here. Should probably not happen in this PR, but I think we could have some shared function that deals with multiple components.
There was a problem hiding this comment.
I can remove and just assume that the graph is connected. Then a composition of graph layouts could be used to discern what happens with connected components.
There was a problem hiding this comment.
Maybe there should be a warning, if the graph isn't connected?
src/KamadaKawai.jl
Outdated
| else | ||
| Vertices=collect(vertices(SubGraph)) | ||
| Vmax=findmax([degree(SubGraph,x) for x in vertices(SubGraph)])[2] | ||
| filter!(x->x!=Vmax, Vertices) |
There was a problem hiding this comment.
Maybe you could use filter!( !isequal(Vmax), Vertices) here or ``filter!( !(==(Vmax)), Vertices)`
src/KamadaKawai.jl
Outdated
| # Currently only LightGraphs are supported using the Dijkstra shortest path algorithm | ||
| K = zeros(N,N) | ||
| for v in 1:N | ||
| K[:,v] = dijkstra_shortest_paths(SubGraph,v).dists |
There was a problem hiding this comment.
I wonder, if we should have the option to input a custom weight matrix.
There was a problem hiding this comment.
Does this also work for directed graphs? (In general, I think for a lot of graph layouts, directed graphs should be seen as as undirected graphs though).
Also, you seem to do that for every vertex? Maybe consider using an algorithm for calculating all shortest paths at the same time, LightGraphs should contain an implementation of floyd-warshall, that could solve that.
There was a problem hiding this comment.
I looked for a call that would do that, but didn't find it. This was expensive, but it works.
I searched explicitly for Floyd-Warshall and it is undocumented, but floyd_warshall_shortest_paths exists. I'll check whether this works as well.
src/KamadaKawai.jl
Outdated
| OptResult = Optim.optimize(x->Objective(x,K),(x,y) -> dObjective!(x,y,K), M0, method=Optim.LBFGS(),iterations = MAXITER ) | ||
| M0 = Optim.minimizer(OptResult) | ||
| min_x, max_x = minimum(M0[1,:]), maximum(M0[1,:]) | ||
| min_y, max_y = minimum(M0[2,:]), maximum(M0[2,:]) |
There was a problem hiding this comment.
Maybe you can us eextrema(M0, dims=2) here. Or minimum(M0, dims=2) and maximum(m0, dims=2).
Otherwise, you should at least use the `@views macro, so that Julia does not allocate space for the subarray.
| min_x, max_x = minimum(locs_x), maximum(locs_x) | ||
| min_y, max_y = minimum(locs_y), maximum(locs_y) | ||
| map!(z -> scaler(z, min_x, max_x), locs_x, locs_x) | ||
| map!(z -> scaler(z, min_y, max_y), locs_y, locs_y) |
There was a problem hiding this comment.
This is probably also some function that we can generalize eventually (also does not have to be in this PR)
src/layout.jl
Outdated
| @@ -192,7 +192,7 @@ Vector of Vector, Vector of node Vector for each shell. | |||
| julia> g = graphfamous("karate") | |||
There was a problem hiding this comment.
We should stop using graphfamous eventually, because it is taken out of this package. The karate graph can then be generated using smallgraphs(:karate). Unfortunately this does not work yet, because we have to wait until a new version of LightGraphs is getting tagged.
There was a problem hiding this comment.
Ah, just realised that was not put in by your pr, so forget about that.
src/KamadaKawai.jl
Outdated
| Interim = filter(x->!(x ∈ VComplement),vcat([collect(neighbors(SubGraph,s)) for s in Shells[end]]...)) | ||
| unique!(Interim) | ||
| push!(Shells,Interim) | ||
| filter!(x->!(x ∈ Shells[end]),Vertices) |
There was a problem hiding this comment.
You can use filter!( !∈(Shells[end]),Vertices) here, or filter!( !in(Shells[end]),Vertices). Unfortunately filter!( ∉(Shells[end]),Vertices) does not work.
src/KamadaKawai.jl
Outdated
| filter!(x->x!=Vmax, Vertices) | ||
| Shells=[[Vmax]] | ||
| VComplement = copy(Shells[1]) | ||
| while length(Vertices)>0 |
There was a problem hiding this comment.
Why not while !isempty(Vertices)
src/KamadaKawai.jl
Outdated
| map!(z -> scaler(z, min_y, max_y), M0[2,:], M0[2,:]) | ||
| locs_x[SubGraphVertices] .= M0[1,:] .+ Offset | ||
| locs_y[SubGraphVertices] .= M0[2,:] | ||
| Offset += maximum(M0[1,:])+C |
There was a problem hiding this comment.
Use the view macro here:
Offset += maximum(@view M0[1,:]) + C
The problem is, that Julia by default does allocate a new Array, when you use a slice (There might be some exceptions to this rule, when using broadcasting, but I am not that familiar yet with that topic).
When you put the @view macro in front, you only get a view of the array instead. This also causes some allocations , where the garbage collector is involved atm, which is actually a bit of a problem when you use it in a loop, but it is still much faster.
There was a problem hiding this comment.
It shouldn't allocate anything here, but since I'm not concerned with the offset in this particular function anymore, I'll just try to remember for future reference ;-)
There was a problem hiding this comment.
Thanks for your contribution, we can definitely need some help for this package.
I did a bit of a shallow review, because I don't find enough time nowadays and I am not yet familiar with layout algorithm and the Optim package.
Currently there are a few things in this package, that are not so optimal, for example we don't really follow the usual conventions for capitalization (Types are upper case, functions and variables are lower case). I was working on this in the past, but did not make any progress in the last few months. So maybe we can already start here .
In the past we have avoided adding to many dependencies to our packages, but as the Optim package seems to be Julia only, I think that could stay (Distances should not be a problem at all). @sbromberger what are your opinions on this?
I will try do have a bit of a deeper look into this PR in the next few days, but as you might have seen from the open PRs and issues, things move a bit slowly at the moment.
Codecov Report
@@ Coverage Diff @@
## master #89 +/- ##
==========================================
- Coverage 32.02% 28.95% -3.08%
==========================================
Files 9 10 +1
Lines 509 563 +54
==========================================
Hits 163 163
- Misses 346 400 +54
Continue to review full report at Codecov.
|
|
I incorporated the comments as seemed appropriate. Optim is part of JuliaOpt. Putting in an independent non-linear optimization implementation is not a good use of time and resources. I haven't added any tests as I am unfamiliar with how that works for visualizations. |
Accidentally had `[compat]` twice in Project.TOML
|
@simonschoelly I've addressed most of your concerns, I belive. Is there anything in particular holding back the inclusion? |
Update to latest base master
Update Kamada Kawai with latest mastr fork
|
@simonschoelly, any thoughts on this PR. It seems that @BCRARL addressed the review points back in 2019. |
Adds
kamada_kawai_layout, which uses the graph-theoretic distance as the equilibrium distance between two vertices. The starting point can be supplied or will be generated from a call toshell_layoutwith shells corresponding to successive next-neighbor shells starting with the highest degree vertex. Disconnected graphs are placed side by side.