Add a mapping from edges to paths with the dummy nodes #6

gsoleilhac · 2020-10-23T12:26:53Z

This makes solve_positions also return a Dict which is a mapping from edges in the original graph to paths in the graph with dummy nodes used to solve the layout problem.

The Dict has for keyset edges(g) and it's values are tuples of vectors : x and y coordinates.

I updated the examples with the output we get by plotting these paths, but the simpler examples don't look so great :

gsoleilhac · 2020-10-23T12:29:52Z

src/layering.jl

-        for out_node in copy(outneighbors(graph, cur_node))  # need to copy as outwise will mutate when the graph is mutated
+        for out_edge in filter(e -> e.src == cur_node, collect(edges(graph)))  # need to copy as outwise will mutate when the graph is mutated


I had to change from iterating the outneighbors to iterating edges to make it work with WeightedGraphs, I couldn't find how to get back to the WeightedEdge with just the src and dst vertices in a generic way. Maybe there's a better way to do it ?

i am not sure. I don't know light graphs that well

Aside: Apparently the rtight way to get the soource is not e.src but rather src(e)

We don't need the collect if we are using filter as filter creates a copy

Apparently the rtight way to get the soource is not e.src but rather src(e)

fixed

We don't need the collect if we are using filter as filter creates a copy

filter on a SimpleEdgeIter throws an MethodError, so I left the collect for now

oxinabox · 2020-10-31T17:40:36Z

src/graph_properties.jl

@@ -1,5 +1,5 @@
 is_dag(g) = is_directed(g) && !is_cyclic(g)
-dag_or_error(g) = is_dag(g) || DomainError(g, "Only Directed Acylic Graphs are supported")
+dag_or_error(g) = is_dag(g) || throw(DomainError(g, "Only Directed Acylic Graphs are supported"))


good catch.

oxinabox · 2020-10-31T17:58:47Z

Why don't we have a function solve_positions_full that returns all the points, and the full graph?
It has all the nodes of the original_graph at the original index, plus all the dummy nodes and their new edges.

then the current solve_positions would just mask-out the dummy-nodes

I don't fully understand this new structure of paths and what it is for.

gsoleilhac · 2020-10-31T18:48:45Z

Well my use case is plotting the output of some maxflow problem.
For each edge in the original graph I want to annotate the plotted edge with the flow passing through it.
If we return the full graph with the new edges, it makes it much harder to do that since I want to annotate a path in the full graph (just once) and not each edge individually.
And then, some information is lost : which edge in the full graph corresponds to which edge in the original graph ?

So that's why I went with a mapping from the edges in the original graph to paths in the full graph

oxinabox · 2020-10-31T21:43:44Z

Ah, I see.
This makes sense now.

Needs doc strings explaining what paths is.

oxinabox · 2020-11-07T23:19:29Z

New docstring looks great.

why is CI failing?
We should have two sets of reference tests.
One set using the coordinates directly and one set using the paths.

gsoleilhac · 2020-11-08T11:33:06Z

Sure,

julia-actions/julia-uploadcoveralls fails on nightly, and that cancels the build on julia stable, but the tests pass successfuly.
Is there an allow failure option on nightly with Github CI ?

oxinabox · 2020-11-08T14:24:56Z

I have added the plots to the readme.
The first one is a good demonstration of the difference this makes.

gsoleilhac · 2020-11-08T18:48:32Z

README.md

+As you can see in the first plot below, pathing though the dummy nodes actually guarantees minimal number of crossings.
+On the _tiny_depgaph_ example direct has 3 crossings, where as path through dummy node only 2).
+However, it also doesn't actually always look as nice.
+It might look as nice if you used some swishy Bezier curves though.


It might look as nice if you used some swishy Bezier curves though.

I just found out that Plots exports curves, with it, the medium_pert example looks like this :

Nice! Shall we change them all to use that?

I don't know, it surely looks better but it doesn't help explaining what is returned in paths

What if we stick a scatter plot on top with big markers dummy nodes,
or insert the letter X in place of the node name for dummy nodes?

Or we could leave as is, maybe adding mention of the curves function in the comment in the readme?

Suggested change

It might look as nice if you used some swishy Bezier curves though.

It might nicer if you used some swishy Bezier curves though (Plots.jl has a [curves attribute](http://docs.juliaplots.org/latest/generated/graph_attributes/), and a matching function, which does this)

oxinabox · 2020-12-29T16:35:29Z

Sorry, I lost track of this.
I think just need to make CI pass (which might mean disabling Julia nightly)
and then we are good to merge.

Also want to decide what to do with curves.
I am now leaning towards just mentioning it in inhe README.md, so we can merge this sooner, rather than having to go through the effort of regenerating the plots.

oxinabox · 2020-12-29T18:07:48Z

Rebasing this should fix the CI

oxinabox · 2021-02-03T14:27:12Z

bump, this would be good to have

gsoleilhac · 2021-02-28T13:29:45Z

Rebased it, sorry for the delay, hard to find motivation these days :)

oxinabox · 2021-02-28T17:28:04Z

Rebased it, sorry for the delay, hard to find motivation these days :)

Big Mood.
(that is what the young people say, right?)

gsoleilhac commented Oct 23, 2020

View reviewed changes

gsoleilhac changed the title ~~Gs/edge to path~~ Add a mapping from edges to paths with the dummy nodes Oct 23, 2020

oxinabox reviewed Oct 31, 2020

View reviewed changes

gsoleilhac commented Nov 8, 2020

View reviewed changes

oxinabox closed this Nov 12, 2020

oxinabox reopened this Nov 12, 2020

oxinabox closed this Dec 29, 2020

oxinabox reopened this Dec 29, 2020

oxinabox closed this Dec 29, 2020

oxinabox reopened this Dec 29, 2020

gsoleilhac added 5 commits February 28, 2021 14:17

returns an additional Dict from edges to paths with the dummy nodes

68314ac

Fix edge type for WeightedGraphs, update readme and demos.jl

3a53bb6

update reference outputs

ad852f0

document solve_positions

ebd89f3

add reference tests with and without paths

1fb2dcd

add new plots to readme

b99ca4e

oxinabox merged commit 5160c38 into oxinabox:main Feb 28, 2021

gsoleilhac deleted the gs/edgeToPath branch March 3, 2021 16:26

oxinabox mentioned this pull request Mar 5, 2021

Remove code duplicated from LayeredLayouts? daschw/SankeyPlots.jl#6

Closed

		for out_node in copy(outneighbors(graph, cur_node)) # need to copy as outwise will mutate when the graph is mutated
		for out_edge in filter(e -> e.src == cur_node, collect(edges(graph))) # need to copy as outwise will mutate when the graph is mutated

	It might look as nice if you used some swishy Bezier curves though.
	It might nicer if you used some swishy Bezier curves though (Plots.jl has a [curves attribute](http://docs.juliaplots.org/latest/generated/graph_attributes/), and a matching function, which does this)

Add a mapping from edges to paths with the dummy nodes #6

Add a mapping from edges to paths with the dummy nodes #6

Uh oh!

Conversation

gsoleilhac commented Oct 23, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oxinabox commented Oct 31, 2020

Uh oh!

gsoleilhac commented Oct 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oxinabox commented Oct 31, 2020

Uh oh!

oxinabox commented Nov 7, 2020

Uh oh!

gsoleilhac commented Nov 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oxinabox commented Nov 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gsoleilhac Nov 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oxinabox Nov 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oxinabox commented Dec 29, 2020

Uh oh!

oxinabox commented Dec 29, 2020

Uh oh!

oxinabox commented Feb 3, 2021

Uh oh!

gsoleilhac commented Feb 28, 2021

Uh oh!

oxinabox commented Feb 28, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

gsoleilhac commented Oct 31, 2020 •

edited

Loading

gsoleilhac commented Nov 8, 2020 •

edited

Loading

oxinabox commented Nov 8, 2020 •

edited

Loading

gsoleilhac Nov 8, 2020 •

edited

Loading

oxinabox Nov 9, 2020 •

edited

Loading