type-based interface #27
Conversation
instead of init/next construct overload Base.iterate directly
Codecov Report
@@ Coverage Diff @@
## master #27 +/- ##
==========================================
- Coverage 98.62% 98.14% -0.48%
==========================================
Files 7 8 +1
Lines 363 431 +68
==========================================
+ Hits 358 423 +65
- Misses 5 8 +3
Continue to review full report at Codecov.
|
|
Docs Preview at https://juliagraphs.org/NetworkLayout.jl/previews/PR27/ |
|
I think this is ready for review. Right now this package does not export any of the layouts, maybe it should? There isn't a lot of testing besides checking the return types of the algorithms. Everything in the docs seems to work fine though... |
Yes, I think we should :) |
SimonDanisch
requested review from
simonschoelly
and removed request for
SimonDanisch
|
One thing i've noticed writing examples for GraphMakie: won't work anymore. 4 options come to mind:
I tend to option 3. Option 2 is also good but I'm not happy with any of the names suggested. Any thoughts on that @SimonDanisch? The second question: the to all of the algorithms (either manually or by using a |
|
|
||
| """ | ||
| AbstractLayout{Dim,Ptype} | ||
|
|
There was a problem hiding this comment.
It would be good to document what the parameters Dim and Pyype are. and what types the can take.
Also thinking about it, how often is the case, that Dim will not be 2 and Ptype not be Float64? Are they really that important that always specify them in the type? Other possibilities would be to specifying these parameters when calling the layout function, maybe as kwargs or just have the default trait functions defined like this
dim(::AbstractLayout) = 2
ptype(::AbstractLayout) = Float64
then one can override these trait functions for a concrete layout, if they would differ somehow,
There was a problem hiding this comment.
The functions dim and ptype aren't used at all in the moment tbh, i am not sure whether they are important at all. But the parameters are used in the layout functions quite often.
Dim: Right now there are 3 layouts which support 2D and 3D (Spring¹, Stress, SFDP), one pure 3D layout (Spectral) and 3 pure 2D layouts (Buchheim, Circular/Shell², SquareGrid). Each layout has the intrinsic property of a dimensionality, I think this justifies to put this information to the supertype. Otherwise we need to add an explicitdimtype parameter to 3 of the layouts (should be a type parameter rather than a field becaus it determines the return valueVector{Point{Dim,Ptype}}of thelayoutfunction).Ptype: Well, the number type of the return values was parametrized before and I did not change that. I'm not gonna lie: this whole interface is opinionated and developed with GraphMakie in mind. And since Makie uses the GPU it is quit common to work withFloat32too.
¹ there is a minor parametrization bug in the 3d spring implementation though but I did not consider this part of the interface
² We don't actually need special code for Circular. The resulting layout of Circular() and Shell() without parameters is the same.
src/buchheim.jl
Outdated
| struct Buchheim{Ptype,T} <: AbstractLayout{2,Ptype} | ||
| nodesize::Vector{T} | ||
| end |
There was a problem hiding this comment.
When we try to separate the layout parameters from the actually layout calculation, does it make sense to store the node/vertex parameters in this struct? This would mean, that we already need to know how many vertices we have, when we create this struct. Maybe it would be better to specify it in
layout(::Buchheim; nodsize=nothing) = ...This way, it might also be easier to deal with graphs that have metadata attached to their nodes later.
There was a problem hiding this comment.
Yes, this is for sure the biggest problem with the proposed interface: it is not completely consistent. There are parameters like the stiffness in spring model or random seeds which are independent of the the the actual layout call. There are other parameters which are tied to stuff like the size of the graph.
I've considered multiple options for this problem:
Layout.layout(g; kwargs...): the old way. This feels very unjulian to me because it defines several functions rather than methods for the same function. Pros: no additional types needed.layout(::Layout, g; kwargs...)whereLayoutonly stores general parameters. Everything else gets passed to thelayoutfunction. I think this is quite confusing because there are several points where to inject parameters. In case of graph plotting on need to either define a closure or add some other container to pass the parameters
lay = g -> layout(Spring(some_para...), g, other_para)
# or
lay = Spring(some_para)
graphplot(g, layout=lay, layoutpara=(other_para...))layout(::Layout, g): all parameters stored inLayoutobject
I went with the third option. Most of the time it does not matter because users will know the size of their graph and this won't change. Wherever possible I've implemented the restrictions very soft: i.e. if there are more nodes than nodesize: just fill up with ones. If there are N nodes but M>N initial positions? Just take the first N etc.
I think this design actually plays very well with stored meta data in case of graph plotting
g = some_meta_graph()
function mylayout(g::MetaGraph)
nodesize = extract_data(g)
return Buchheim(; nodesize)(g)
end
plot(g; layout=mylayout)There was a problem hiding this comment.
After a short discussion with SimonD i've removed the layout function from the exported interface. For one-shot application there are now lower case function calls for all of the layouts. So now there are two main ways of applying layouts:
layoutfunction = MyLayout(; kwargs) # creates callable object to apply layout later
positions = mylayout(g; kwargs...) # apply layout right nowthe second one is quite similar to the old Spring.layout(g; kwargs...).
src/squaregrid.jl
Outdated
| `skip=[(i,j)]` means to keep the position in the `i`-th row and `j`-th column | ||
| empty. | ||
| """ | ||
| struct SquareGrid{Ptype,CT} <: AbstractLayout{2,Ptype} |
There was a problem hiding this comment.
To be honest, I am a bit unsure what the purpose of this layout is, does it really make sense to line up the vertices row by row on a grid? Won't this cause a huge mess when plotting the graph with overlapping edges?
There was a problem hiding this comment.
Well this one is quite specific for my usecase to be honest. In papers about networks with geographic/planar embedding (for example power grids) it is quite common to use square grids as toy models. The example in the docs is bad though.
(Franz Kaiser et al 2020 New J. Phys. 22 013053)
(Manik, D., Molkenthin, N. Topology dependence of on-demand ride-sharing. Appl Netw Sci 5, 49 (2020). https://doi.org/10.1007/s41109-020-00290-2)
Co-authored-by: Simon Schölly <[email protected]>
- the layout export blocks identifier in global scope - new lowercase functions are easy to read for singe application i.e. `spring(g; kwargs...)` instead of `Spring(;kwargs)(g)`
This WIP PR should resolve #25. The main goal is to split the parametrisation of each algorithm from the actual layout call.
The new public interface is as follows. Each algorithm/layout has a struct which holds the algorithm parameters.
Each
AbstractLayoutis callable to retrieve the positions for a adjacency matrixFor iterative layouts one can get access to the intermediate states
assert_squareCircular?SquareGrid?PtypeandDimspring,buchheim, ... methodslayoutfrom exports