Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow non-contiguous layers to be specified simultaneously in force_layer and force_equal_layers #43

Merged
merged 5 commits into from
Jun 10, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Project.toml
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
name = "LayeredLayouts"
uuid = "f4a74d36-062a-4d48-97cd-1356bad1de4e"
authors = ["Frames White <[email protected]> and contributors"]
version = "0.2.9"
version = "0.2.10"

[deps]
Dates = "ade2ca70-3891-5945-98fb-dc099432e06a"
Expand Down
2 changes: 1 addition & 1 deletion src/graph_properties.jl
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ sinks(graph) = findall(iszero, outdegree(graph))

function longest_paths(graph, roots)
dag_or_error(graph)
dists = zeros(nv(graph))
dists = zeros(Int, nv(graph))
pending = [0 => r for r in roots]
while(!isempty(pending))
depth, node = pop!(pending)
Expand Down
3 changes: 2 additions & 1 deletion src/layering.jl
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@
function layer_by_longest_path_to_source(graph, force_layer)
dists = longest_paths(graph, sources(graph))
force_layers!(graph, dists, force_layer)
layer_groups = collect.(IterTools.groupby(i->dists[i], sort(vertices(graph), by=i->dists[i])))
layer_groups = [findall(==(d), dists) for d in 1:maximum(dists)]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note the different time complexity.
This is O(n*d) where as the previous code is O(n* log(n))

But it seems we can do better with

Suggested change
layer_groups = [findall(==(d), dists) for d in 1:maximum(dists)]
layer_groups = [Int[] for d in 1:maximum(dists)]
for (ii, d) in enumerate()
push!(layer_groups[d], ii)
end

Which is only O(n + d)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I hadn't thought this. Your suggestion is very nice: I will add that now 👍 !

Copy link
Contributor Author

@thchr thchr Jun 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be more natural to simply make this a Dict{Int, Vector{Int}} instead? Then there could also be negative indices and we wouldn't have to potentially store a bunch of empty layers.

Maybe there's some assumptions in the code elsewhere that assumes iteration over layer2nodes in incrementing fashion?

Copy link
Owner

@oxinabox oxinabox Jun 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be more natural to simply make this a Dict{Int, Vector{Int}} instead? Then there could also be negative indices and we wouldn't have to potentially store a bunch of empty layers.

My instinct is that in practical cases the extra costs from empty layers will be lower than the extra cost of needing to do a hash.

Where would we get negative layer indexes? Would that be just from users specifying them by hand?
So it would be a way to say to put this before the natural root layer?
That sounds like a bigger change that would want to be in its own PR

Maybe there's some assumptions in the code elsewhere that assumes iteration over layer2nodes in incrementing fashion?

Good question. I don't think so, but i can't from memory rule it out

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, better to leave that for a potential follow-up, if ever relevant. Thanks for expeditious merging 👍 !


return layer_groups
end

Expand Down
1 change: 1 addition & 0 deletions src/zarate.jl
Original file line number Diff line number Diff line change
Expand Up @@ -278,6 +278,7 @@ function assign_coordinates(layout, graph, layer2nodes;

node2y = Dict{Int, VariableRef}()
for (layer, nodes) in enumerate(layer2nodes)
isempty(nodes) && continue # skip empty layers
first_node, other_nodes = Iterators.peel(nodes)
prev_y = node2y[first_node] = @variable(m, base_name="y_$first_node")
for node in other_nodes
Expand Down
13 changes: 13 additions & 0 deletions test/demos.jl
Original file line number Diff line number Diff line change
Expand Up @@ -78,4 +78,17 @@ end
test_example(layout, :two_lines_flipped_vertex_order; force_equal_layers=[1=>3])
#test_example(layout, :large_depgraph) # too big
#test_example(layout, :extra_large_depgraph) # too big

# laying out a graph with specified layering values - which intentionally skips the 16th
# layer entirely - while also using `force_equal_layers` relative to this layering set
test_example(layout, :disconnected_components_graph;
force_layer = [ # note that the 16th layer is skipped
1=>1, 2=>1, 3=>2, 4=>2, 5=>3, 6=>4, 7=>4, 8=>5, 9=>6, 10=>6, 11=>7, 12=>7,
13=>8, 14=>8, 15=>9, 16=>9, 17=>10, 18=>10, 19=>11, 20=>12, 21=>12, 22=>13,
23=>14, 24=>14, 25=>15, 26=>15, 27=>17, 28=>18, 29=>18, 30=>19, 31=>19, 32=>20,
33=>20, 34=>21, 35=>22, 36=>22, 37=>23, 38=>24, 39=>24, 40=>25, 41=>25, 42=>26,
43=>26, 44=>27, 45=>27, 46=>28, 47=>28, 48=>29, 49=>30, 50=>30, 51=>31, 52=>31],
force_equal_layers = [
1=>7, 1=>9, 1=>15, 3=>5, 3=>11, 3=>13, 17=>21, 17=>23, 17=>29, 19=>25, 19=>27,
19=>31])
end
60 changes: 60 additions & 0 deletions test/examples.jl
Original file line number Diff line number Diff line change
Expand Up @@ -488,6 +488,66 @@ module Examples
2 => 7
2 => 24
]))

# a graph with disconnected components
disconnected_components_graph = SimpleDiGraph(Edge.([
1 => 3
2 => 4
3 => 5
4 => 5
5 => 6
5 => 7
6 => 8
7 => 8
8 => 9
8 => 10
9 => 11
10 => 12
11 => 13
12 => 14
13 => 16
14 => 15
15 => 18
16 => 17
17 => 19
18 => 19
19 => 20
19 => 21
20 => 22
21 => 22
22 => 23
22 => 24
23 => 26
24 => 25
27 => 28
27 => 29
28 => 30
29 => 31
30 => 32
31 => 33
32 => 34
33 => 34
34 => 35
34 => 36
35 => 37
36 => 37
37 => 38
37 => 39
38 => 41
39 => 40
40 => 43
41 => 42
42 => 44
43 => 45
44 => 46
45 => 47
46 => 48
47 => 48
48 => 49
48 => 50
49 => 52
50 => 51
]))
end # module


Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading