JuliaGraphs / NetworkLayout.jl

Layout algorithms for graphs and trees in pure Julia.
Other
96 stars 22 forks source link

adding fixed locations to SFDP - update #52

Closed claytonpbarrows closed 1 year ago

claytonpbarrows commented 2 years ago

this is a redo on #29 based on v0.4

thchr commented 2 years ago

I would love to use this!

I wonder though if the interface could be designed to allow greater flexibility in what to "fix": e.g., it might be nice to be able to fix just a single coordinate of a node (i.e. x or y coordinate) in addition to specifying both coordinates simultaneously.

Tentatively, could the interface be a Vector or Dict of Vector{Union{Nothing, <:Real}} instead? EDIT: I think aninterface where the "pinning"/"fixing" is done via a variable pin :: Union{Nothing, Vector{Point{Dim,Bool}}} could work well, with pinning of the jth coordinate of the ith position decided by the value of pin[i][j]. To disable any pinning whatsoever, one could set pin to nothing.

claytonpbarrows commented 2 years ago

In the current implementation, and in the suggested change where pinning is done via a variable pin :: Union{Nothing, Vector{Point{Dim,Bool}}}, the adjacency matrix must be sorted so that the n edges with n known positions occur first. That is consistent with the initialpos implementation. Is that what you're imagining here?

thchr commented 2 years ago

I was thinking that the vertex iterator of the graph and pin should just have identical lengths. It's true that it might be nice to allow pin to potentially just have the length of initialpos though.

Without the initialpos-length complication, we could do this by adding a pin::Union{Nothing, Vector{Point{Dim,Bool}}} field to SFDP and then changing the location update to:

norm_force = norm(force)
if !isnothing(algo.pin)
    locs[i] += (!).(algo.pin[i]) .* (force * (step/norm_force))
else
    locs[i] += force * (step/norm_force)
end

If we allow pin's length to be shorter than the number of vertices (but equal to initialpos), then we'd need a length check like what you currently have as well.

thchr commented 2 years ago

Tangentially, it would be nice to add this functionality to Spring as well (which has a very similar update step). Might be that Stress could have this too, but the update step is a bit different...

hexaeder commented 2 years ago

I'd also really prefere to find a nice interface which could be added to all the interative layouts. I am not plotting that many graphs nowadays but when i wanted to use this feature I was allways thinking of an dict based interface. Something like pin = Dict{Int, Point}(2 => (1,0), 7=>(-1,-1) which could be used to initialise the position and tell the algorithm wich vertices to pin at the same time

claytonpbarrows commented 2 years ago

@hexaeder I like your interface improvement suggestion. I'd suggest that the same interface should be used for initialpos and pin. Do you agree?

hexaeder commented 2 years ago

I think there is an important usage difference between initialpos and pin: If you want to give a special initialisation for the algorithm you're likely to do so for the whole graph, then the vector makes sense [1]. At least for my usage pin is different in that regard: normally I want to place a few key vertices at some positions and the rest will should group arround them according to the algorithm.

However, as long as it is non-breaking it would be cool if initialpos could handle both? Something like

struct Spdf{..., IPT} where {IPT <: Union{Dict, Vector}}
     initialpos::IPT
end

# and during construction
startpos = rand(N)
for (k,v) in pairs(algo.initialpos)
    startpos[k] = v
end

[1] The fact that you may specify the first M initial positions of a graph with N != M vertices is a more or less unintended side effect of the interface. For GraphMakie i wanted to fix layout parameters before actual applying the layout to a specific graph. To increase flexibility I chose it in a way, that you can specify 10 initial positions and the algorithm will still produce layouts for graphs with 9 or 11 vertices...

claytonpbarrows commented 2 years ago

This latest attempt gets most of the way towards the suggestions from @hexaeder To summarize:

I don't think this quite meets all the requests. And it's not the cleanest implementation, but I've spent too much time on this now and I have to put it down.

codecov[bot] commented 2 years ago

Codecov Report

Merging #52 (43f732a) into master (8eca84d) will decrease coverage by 0.67%. The diff coverage is 88.88%.

@@            Coverage Diff             @@
##           master      #52      +/-   ##
==========================================
- Coverage   97.44%   96.76%   -0.68%     
==========================================
  Files           8        8              
  Lines         470      526      +56     
==========================================
+ Hits          458      509      +51     
- Misses         12       17       +5     
Impacted Files Coverage Δ
src/sfdp.jl 95.40% <88.88%> (-2.91%) :arrow_down:
src/spring.jl 96.00% <0.00%> (-1.78%) :arrow_down:
src/stress.jl 96.93% <0.00%> (-0.82%) :arrow_down:
src/shell.jl 100.00% <0.00%> (ø)
src/squaregrid.jl 100.00% <0.00%> (ø)
src/buchheim.jl 97.32% <0.00%> (+0.13%) :arrow_up:
src/spectral.jl 96.15% <0.00%> (+0.15%) :arrow_up:
src/NetworkLayout.jl 94.73% <0.00%> (+0.29%) :arrow_up:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

hexaeder commented 1 year ago

closed by #53