-
Notifications
You must be signed in to change notification settings - Fork 28
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
Added functionality to save graphs in DOT format #23
Conversation
Codecov Report
@@ Coverage Diff @@
## master #23 +/- ##
========================================
- Coverage 99.67% 83.67% -16%
========================================
Files 10 11 +1
Lines 304 435 +131
========================================
+ Hits 303 364 +61
- Misses 1 71 +70
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #23 +/- ##
==========================================
- Coverage 99.67% 91.62% -8.06%
==========================================
Files 10 11 +1
Lines 304 358 +54
==========================================
+ Hits 303 328 +25
- Misses 1 30 +29
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good thing we get this functionality. As you see, you are getting some warnings. This is, because DOTFormat
has been moved to the submodule GraphIO.DOT.DOTFormat
. So you either have to write DOT.DOTFormat
. or using GraphIO.DOT: DOTFormat
.
I wonder if this would be a good opportunity to add the ability to add the ability to attach labels to vertices and edges, as this is not too difficult with this format. We could do this with optional keyword arguments. What do you think @sbromberger ?
src/DOT/Dot.jl
Outdated
continue | ||
end | ||
s = string(n) | ||
edg = edg * "\n\t" * string(u) * " -> {" * s[2:length(s)-1] * "}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thats a clever trick, but the output of string(n)
changes slightly in the future, this might break. consider doing something like join(map(string, outneighbors(g, u)), ", ")
instead.
src/DOT/Dot.jl
Outdated
edg = "" | ||
if isdir | ||
for u in LightGraphs.vertices(g) | ||
n = LightGraphs.outneighbors(g, u) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
n
is not a good name here, usually people assume that this would be an integer.
src/DOT/Dot.jl
Outdated
for e in LightGraphs.edges(g) | ||
source = string(LightGraphs.src(e)) | ||
dest = string(LightGraphs.dst(e)) | ||
edg = edg * "\n\t" * source * " -- " * dest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem here is that you use *
to build a large string. This creates a lot of unnecessary allocations and will become really slow when the graph gets big. Its better to work with IOBuffers or write each line directly to io
as soon as you have finished creating it. In fact you can use println(io, "some string")
, so you will not have to insert \n
manually.
using ParserCombinator,LightGraphs,GraphIO
x = SimpleGraph(5,10)
y = SimpleDiGraph(5,2)
savegraph("/home/abhinav/undirected.dot",x,"undg",GraphIO.DOT.DOTFormat())
savegraph("/home/abhinav/directed.dot",y,"dg",GraphIO.DOT.DOTFormat())
p = loadgraph("/home/abhinav/undirected.dot","undg",GraphIO.DOT.DOTFormat())
q = loadgraph("/home/abhinav/directed.dot","dg",GraphIO.DOT.DOTFormat())
println(p == x)
println(q == y) Output - true
true |
I think this might be better as a method for |
Hi, I'm not a contributor here, but I'm waiting for this functionality to be added and I just took a look on the code you've submitted. This is a fairly straightforward functionality and I'm wondering why you keep all the logic in a single function. In line |
There are two reasons, why I think it might be a good idea to do that here:
|
We really should make some progress here. Lets skip the issue with the metadata, that can be added later. I think in your current code, isolated vertices will not be added to the dotfile? Can you fix that? |
Isolated vertices are added. The problem is that when you save the DOT file and load it again the graphs will not be the same because of this - Line 49 in b79931c
|
You are right, I totally forgot about that. But your code outputs the graph in the right order and it is the importer that messes that up? Because in that case, we don't have to fix it here. |
Can you add some tests? We can get around the issue that the vertex order is messed up in the importer by either using vertex-transitive graphs for testing or by using the isomorphism algorithms for reordering. |
Yes the DOT parser returns nodes in random order, so the loaded graph is not the same but yes the saved graph is correct. |
If I understand correctly does that mean checking if the loaded graph is isomorphic to the original saved graph in the tests ? But the problem will still remain, right ? I mean is it only for testing ? |
We could fix this problem for LightGraphs only btw, like b79931c I was doing before, but then it is not generalized. |
Yes, only for testing. Maybe also add a comment there, why we are doing this. Let's think about what would be correct for the importer in another issue/pr, so we can finally merge this. |
Okay I'll add some tests then. |
I have added the tests but code coverage seems to be wrong. |
Ouch, this got completely forgotten. Maybe you can implement that thing that I suggested and then rebase on master. Then lets see if code coverage still complains.. |
Co-Authored-By: Simon Schoelly <[email protected]>
The diff coverage seems fine. |
Are we good to merge this? |
#19