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

Export graph to file #18828

Closed
nathanncohen mannequin opened this issue Jun 30, 2015 · 26 comments
Closed

Export graph to file #18828

nathanncohen mannequin opened this issue Jun 30, 2015 · 26 comments

Comments

@nathanncohen
Copy link
Mannequin

nathanncohen mannequin commented Jun 30, 2015

I feel rather guilty, as all that this branch does is steal networkx functions. With it we can export graphs to several file formats, using the networkx.write_* functions.

Nathann

CC: @dimpase @dcoudert @sagetrac-borassi

Component: graph theory

Author: Nathann Cohen

Branch/Commit: 254b36c

Reviewer: David Coudert

Issue created by migration from https://trac.sagemath.org/ticket/18828

@nathanncohen nathanncohen mannequin added this to the sage-6.8 milestone Jun 30, 2015
@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Jun 30, 2015

Commit: 1bd29e5

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Jun 30, 2015

Branch: u/ncohen/18828

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Jun 30, 2015

New commits:

1bd29e5trac #18828: Export graph to file

@nathanncohen nathanncohen mannequin added the s: needs review label Jun 30, 2015
@dcoudert
Copy link
Contributor

comment:3

Hello,

The patchbot reports compilation errors associated with ticket #18746. I don't know why...
Actually I have the same issue when I try to compile my develop branch, and so I'm currently unable to test this patch.

Concerning this ticket. Don't feel guilty to steal networkx. It is very useful and we will certainly also have to do the same for reading graphs from file.

I suggest to change the way you guess the file format with something like:

for ext in formats:
    if filename.endswith('.'+ext):
        break
finally:
    raise ...

With networkx it is possible to give extension like .edgelist.gz in which case the file should be compressed (at least with version 1.9.1, but we only have 1.8.1). See https://networkx.github.io/documentation/latest/reference/generated/networkx.readwrite.edgelist.write_edgelist.html

David.

@sagetrac-borassi
Copy link
Mannequin

sagetrac-borassi mannequin commented Jul 1, 2015

comment:4

Hellooooo!

I have a little trouble understanding the goal of this ticket: would you like to have only one standard function to save graphs, instead of calling different NetworkX functions, right? Because, for instance, if I want to save a file in adjlist format I can simply type:

import networkx
networkx.write_adjlist(G, path)

instead of using this new function.

In any case, I have tried to compile this code, both with make and with make distclean && make, and it works! Some small suggestions:

  • could you make a test that checks if the output is correct?

  • "the format is ‘guessed’ from the extension ..." maybe it is better to say "the format is the extension", because you do not guess, you simply use the extension.

  • I would add a link to the NetworkX manual (!http://networkx.lanl.gov/reference/readwrite.html), where the different file formats are detailed.

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Jul 1, 2015

comment:5

Hello,

The patchbot reports compilation errors associated with ticket #18746. I don't know why...
Actually I have the same issue when I try to compile my develop branch, and so I'm currently unable to test this patch.

I had the same problem. Can be solved by removing all the cython cached files that you can find:

./src/build/temp.linux-x86_64-2.7/sage/graphs/graph_decompositions
./src/build/temp.linux-x86_64-2.7/home/ncohen/.Sage/src/build/cythonized/sage/graphs/graph_decompositions
./src/build/cythonized/sage/graphs/graph_decompositions
./src/build/cython_debug/cython_debug_info_sage.graphs.graph_decompositions*
./src/build/lib.linux-x86_64-2.7/sage/graphs/graph_decompositions
./local/lib/python2.7/site-packages/sage/graphs/graph_decompositions

And then it works. That's trouble for the patchbots, though.

Concerning this ticket. Don't feel guilty to steal networkx. It is very useful and we will certainly also have to do the same for reading graphs from file.

Yep.

I suggest to change the way you guess the file format with something like:

for ext in formats:
    if filename.endswith('.'+ext):
        break
finally:
    raise ...

Why? It is longer, and does the same. None of the extensions contains a point.

With networkx it is possible to give extension like .edgelist.gz in which case the file should be compressed (at least with version 1.9.1, but we only have 1.8.1). See https://networkx.github.io/documentation/latest/reference/generated/networkx.readwrite.edgelist.write_edgelist.html

So you want to add all combinations of .edgelist.gz, edgelist.tgz,.. for all possible combinations to the dictionary of extensions? O_o

Nathann

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Jul 1, 2015

comment:6

Hello,

I have a little trouble understanding the goal of this ticket: would you like to have only one standard function to save graphs, instead of calling different NetworkX functions, right?

Yes

Because, for instance, if I want to save a file in adjlist format I can simply type:

That's what the branch does, too. Only you may not know that those functions can be found in networkx.

In any case, I have tried to compile this code, both with make and with make distclean && make, and it works! Some small suggestions:

  • could you make a test that checks if the output is correct?

I added one check, but it is unpleasant in many ways. First, the (integer) vertices become strings, and then each edge gets a label encoding a weight. Well, that's networkx...

  • "the format is ‘guessed’ from the extension ..." maybe it is better to say "the format is the extension", because you do not guess, you simply use the extension.

How is that not a guess? Anyway, udpated.

Done.

Nathann

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 1, 2015

Changed commit from 1bd29e5 to f7c4a18

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 1, 2015

Branch pushed to git repo; I updated commit sha1. New commits:

f7c4a18trac #18828: Reviewer's remarks

@dcoudert
Copy link
Contributor

dcoudert commented Jul 1, 2015

comment:9

This method is clearly useful. It is too boring to import networkx each time you want to read/write a graph from/to a file.

  • could you make a test that checks if the output is correct?

I added one check, but it is unpleasant in many ways. First, the (integer) vertices become strings, and then each edge gets a label encoding a weight. Well, that's networkx...

By default method networx.write_edgelist sets parameter data=True. So your method produce a weighted edgelist instead of an edgelist

0 1 {}

instead of

0 1

I understand that you prefer short and fast code, but since writing to a file is slow anyway, we could spend some computation time to refine the behavior of the method.

David

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Jul 1, 2015

comment:10

I understand that you prefer short and fast code, but since writing to a file is slow anyway, we could spend some computation time to refine the behavior of the method.

Would it do the trick for you if we replaced 'networkx.write_edgelist' with 'lambda x:networkx.write_edgelist(x,data=False)'?

Nathann

@dcoudert
Copy link
Contributor

dcoudert commented Jul 1, 2015

comment:11

Replying to @nathanncohen:

I understand that you prefer short and fast code, but since writing to a file is slow anyway, we could spend some computation time to refine the behavior of the method.

Would it do the trick for you if we replaced 'networkx.write_edgelist' with 'lambda x:networkx.write_edgelist(x,data=False)'?

Nice trick. Furthermore, in case we want to write the labels, we can have another parameter to set data=True.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 1, 2015

Branch pushed to git repo; I updated commit sha1. New commits:

c019634trac #18828: Expose all options from networkx

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 1, 2015

Changed commit from f7c4a18 to c019634

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Jul 1, 2015

comment:13

Nice trick. Furthermore, in case we want to write the labels, we can have another parameter to set data=True.

Some functions have this 'data' flag, others do not. To make everything simpler I updated the code to make it possible to forward any other flag to networkx. This way, we can pick whatever we want.

Nathann

@dcoudert
Copy link
Contributor

dcoudert commented Jul 2, 2015

comment:14

I propose to add extend the tests in the following way. Let me know if you agree before I push a commit.

sage: g = graphs.PetersenGraph()
sage: filename = tmp_filename(ext=".pajek")
sage: g.export_to_file(filename)
sage: import networkx
sage: h = Graph( networkx.read_pajek(filename)
sage: g.is_isomorphic(h)
True
sage: filename = tmp_filename(ext=".edgelist")
sage: g.export_to_file(filename, data=False)
sage: h = Graph( networkx.read_edgelist(filename)
sage: g.is_isomorphic(h)
True

Relying on vertex names is unfortunately not possible yet since the read method turns vertex id like 13 to u'13'.

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Jul 2, 2015

comment:15

No problem no problem. Or perhaps we could relabel the graph with the function 'int' ? This should turn the u'13' into a proper 13.

Nathann

@dcoudert
Copy link
Contributor

dcoudert commented Jul 3, 2015

Changed branch from u/ncohen/18828 to u/dcoudert/18828

@dcoudert
Copy link
Contributor

dcoudert commented Jul 3, 2015

comment:17

I have pushed a small edit on the example. Hope it helps.

For the unicode problem, I have once used the following trick. Certainly not the best way to do it.

import string

if all(isinstance(u, unicode) for u in G):
    myaction = string.atoi
elif all(isinstance(u, str) for u in G):
    myaction = ZZ
else:
    myaction = lambda x:x

try:
    L = {u:myaction(u) for u in G}
except:
    L = {u:str(u) for u in G}

G.relabel(perm=L, inplace=True)

New commits:

fed2fd7trac #18828: Merged with 6.8.beta7
254b36ctrac #18828: fix and improve test/examples

@dcoudert
Copy link
Contributor

dcoudert commented Jul 3, 2015

Changed commit from c019634 to 254b36c

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Jul 3, 2015

comment:18

Works for me !

@dcoudert
Copy link
Contributor

dcoudert commented Jul 3, 2015

comment:19

So then good to go.

It would be nice to have a method for importing a graph from file, but I don't know where to put it: as a method of class Generic_Graph? as a method of (di)graphs generators?

@dcoudert
Copy link
Contributor

dcoudert commented Jul 3, 2015

Reviewer: David Coudert

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Jul 3, 2015

comment:20

It would be nice to have a method for importing a graph from file, but I don't know where to put it: as a method of class Generic_Graph? as a method of (di)graphs generators?

The 'classiest way' would probably be something like Graph(filename). But adding formats to the current list scares me :-P

Nathann

@dcoudert
Copy link
Contributor

dcoudert commented Jul 3, 2015

comment:21

The 'classiest way' would probably be something like Graph(filename). But adding formats to the current list scares me :-P

I understand. Although you did an impressive cleaning, it's still hard.
Well, we will think to something.

@vbraun
Copy link
Member

vbraun commented Jul 3, 2015

Changed branch from u/dcoudert/18828 to 254b36c

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants