GraphMakie update #1115
GraphMakie update #1115
Conversation
|
The first and third look great! The second is a bit strange. Do you want to tweak the layouts more in the PR, or get it merged as is and update later? (I ask as I'll review it sooner in the latter case, or wait till you say you are done in the former case). |
|
Also, is this based on the current master? I thought I had removed BifurcationKit there so the docs build but this seems to be crashing on it. |
|
Probably just merge this one as is and I'll look more at it in a follow-up. Also just rebased it, it seems like the remote branch was based on an earlier master commit |
|
It may still fail -- I have another PR that is completely removing Bifkit, so it may need one more rebasing once that is merged (but if tests pass and docs build as is I'll merge after reviewing). |
| import Graphs: AbstractGraph, SimpleGraph, SimpleDiGraph, SimpleEdge, src, dst, ne, nv | ||
| import Catalyst: speciesreactiongraph, incidencematgraph |
There was a problem hiding this comment.
Why import and not using? Generally I thought there isn't much reason to import unless adding an unqualified dispatch, i.e. src(rn::ReactionSystem) vs. Graphs.src(rn::ReactionSystem). And if you are dispatching I think it is better to qualify with the package name anyways (i.e. Graph.src) (though we haven't been good about this in the past).
| # Create the SimpleDiGraph corresponding to the species and reactions | ||
| function SRGraphWrap(rn::ReactionSystem) | ||
| srg = speciesreactiongraph(rn) | ||
| rateedges = Vector{SimpleEdge}() | ||
| sm = speciesmap(rn); specs = species(rn) | ||
|
|
||
| for (i, rx) in enumerate(reactions(rn)) | ||
| deps = get_variables(rx.rate, specs) | ||
| if deps != Any[] | ||
| for spec in deps | ||
| specidx = sm[spec] | ||
| push!(rateedges, SimpleEdge(specidx, i + length(specs))) | ||
| end | ||
| end | ||
| end | ||
| SRGraphWrap(srg, rateedges) | ||
| end |
There was a problem hiding this comment.
This should probably go right after the type def since it is a constructor for the type.
| end | ||
| speciesreactiongraph(sir) | ||
| """ | ||
| function speciesreactiongraph(rn::ReactionSystem) |
There was a problem hiding this comment.
| function speciesreactiongraph(rn::ReactionSystem) | |
| function species_reaction_graph(rn::ReactionSystem) |
Getting too long not to split probably.
| adjmat = zeros(Int64, nv, nv) | ||
| for (i, rx) in enumerate(rxs) | ||
| for (spec, stoich) in zip(rx.substrates, rx.substoich) | ||
| adjmat[sm[spec], s+i] = stoich | ||
| end | ||
| for (spec, stoich) in zip(rx.products, rx.prodstoich) | ||
| adjmat[s+i, sm[spec]] = stoich | ||
| end | ||
| end |
There was a problem hiding this comment.
Can you not just collect the index pairs and build an edge list? The matrix doesn't seem needed, and the edge list approach should work for large sparse systems too.
| - The `interactive` flag sets the ability to interactively drag nodes and edges in the generated plot. | ||
| Only allowed if `GLMakie` is the loaded Makie backend. | ||
| """ | ||
| function ComplexGraph(rn::ReactionSystem; interactive = false) |
There was a problem hiding this comment.
| function ComplexGraph(rn::ReactionSystem; interactive = false) | |
| function plot_complex_graph(rn::ReactionSystem; interactive = false) |
Since this isn't a struct we should use a more standard function naming style.
| push!(labels, complexelemtostr(complex[1], specstrs)) | ||
| else | ||
| elems = map(c -> complexelemtostr(c, specstrs), complex) | ||
| str = reduce((e1, e2) -> *(e1, " + ", e2), elems[2:end]; init = elems[1]) |
There was a problem hiding this comment.
| str = reduce((e1, e2) -> *(e1, " + ", e2), elems[2:end]; init = elems[1]) | |
| str = reduce((e1, e2) -> *(e1, " + ", e2), @view elems[2:end]; init = elems[1]) |
| - The `interactive` flag sets the ability to interactively drag nodes and edges in the generated plot. | ||
| Only allowed if `GLMakie` is the loaded Makie backend. | ||
| """ | ||
| function SRGraph(rn::ReactionSystem; interactive = false) |
There was a problem hiding this comment.
| function SRGraph(rn::ReactionSystem; interactive = false) | |
| function plot_speciesreaction_graph(rn::ReactionSystem; interactive = false) |
| elist[i] == elist[i-1] && begin | ||
| edgecolors[i] = :red | ||
| insert!(edgelabels, i, "") | ||
| end |
There was a problem hiding this comment.
| elist[i] == elist[i-1] && begin | |
| edgecolors[i] = :red | |
| insert!(edgelabels, i, "") | |
| end | |
| if elist[i] == elist[i-1] | |
| edgecolors[i] = :red | |
| insert!(edgelabels, i, "") | |
| end |
| for (i, rx) in enumerate(reactions(rn)) | ||
| deps = get_variables(rx.rate, specs) | ||
| if deps != Any[] |
There was a problem hiding this comment.
| for (i, rx) in enumerate(reactions(rn)) | |
| deps = get_variables(rx.rate, specs) | |
| if deps != Any[] | |
| deps = Set() | |
| for (i, rx) in enumerate(reactions(rn)) | |
| empty!(deps) | |
| get_variables!(deps, rx.rate, specs) | |
| if !isempty(deps) |
| edgeorder = sortperm(edgelist) | ||
| edgelist = edgelist[edgeorder] |
Refactoring the Graphviz code into GraphMakie. Some plots generated:
Brusselator species-reaction graph
Brusselator complex graph
Repressilator species-reaction graph
Might be worth playing around with the layouts a bit more to see if we can make the edge-lengths for these graphs more consistent and have the layouts be more horizontal. Will work on updating the documentation in a follow-up.