Opened 4 years ago
Closed 4 years ago
#18375 closed enhancement (fixed)
Drop the NetworkX graph backend
Reported by:  ncohen  Owned by:  

Priority:  major  Milestone:  sage6.7 
Component:  graph theory  Keywords:  
Cc:  dcoudert, borassi, vdelecroix  Merged in:  
Authors:  Nathann Cohen  Reviewers:  David Coudert 
Report Upstream:  N/A  Work issues:  
Branch:  98c86a4 (Commits)  Commit:  98c86a41330a3cffacbedc1743467db9aa08117b 
Dependencies:  Stopgaps: 
Description
This branch deprecates the NetworkX
backend for graphs. Indeed, while it made
sense to keep a NetworkXGraphBackend
when Sage's default data structure
switched from networkx
to c_graphs
, we do not *need* it anymore.
This does not mean that Sage should not have a NetworkX
data structure (though
there is little use). On the other hand, NetworkX
is the only reason why we
have a Backend
layer in Sage's graph data structures, as all others are
CGraph
backends.
Thus, in order to simplify the hierarchy of classes for graph data structures,
it is easier to uniformize our data structures first. When this series of
patches will be finished and when the graph backends will be in a clearer and
simpler state, it will be possible to implement a new NetworkX
backend.
It makes little sense, however, to rewrite the current networkx backend as a
c_graph
backend when it will have to be rewritten again later during the
refactoring.
Technical info:
 The main problem in this branch was the handling of (old) graph pickles. As
the classes themselves are being deprecated, the
__setstate__
functions detect whenever an attribute uses a deprecated class, and in this case convert it into a nondeprecated data structure (sparse graph, by default).
 Two
random_stress
functions are removed, as they work by comparing the behaviour ofc_graph
andnetworkx
backend. This is not as terrible as it sounds, for graphs are tested extensively in many places of Sage (I am often surprised to find out where they are used when code breaks)
Change History (17)
comment:1 Changed 4 years ago by
 Branch set to public/18375
 Cc dcoudert borassi vdelecroix added
 Status changed from new to needs_review
comment:2 Changed 4 years ago by
 Commit set to d5c870a03e851e8b2bb4b9defd77083cc6847a91
comment:3 Changed 4 years ago by
 Commit changed from d5c870a03e851e8b2bb4b9defd77083cc6847a91 to 26c47897f8b8312baf2878f323e9d7fe5541c98a
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
26c4789  trac #18375: Drop the NetworkX graph backend

comment:4 followup: ↓ 5 Changed 4 years ago by
You do not like these random_stress
functions?
comment:5 in reply to: ↑ 4 Changed 4 years ago by
You do not like these
random_stress
functions?
Not much. They could have used more doc. But more importantly, they do not work without a networkx backend.
comment:6 followup: ↓ 9 Changed 4 years ago by
I did long tests on the graph module and it is OK. The doc also builds properly.
Should I test something else to ensure that this patch is working well? In particular, how can I test the pickle stuff.
comment:7 followup: ↓ 8 Changed 4 years ago by
Dumb questions that I get scared about when I see this ticket: What if people are working with NX stuff and want to use them in Sage easily? Does this make this impossible? Also, are you suggesting dropping NX as a standard package (assuming it still is one)?
comment:8 in reply to: ↑ 7 ; followup: ↓ 16 Changed 4 years ago by
Dumb questions that I get scared about when I see this ticket: What if people are working with NX stuff and want to use them in Sage easily? Does this make this impossible? Also, are you suggesting dropping NX as a standard package (assuming it still is one)?
This ticket removes a feature you probably did not know even existed:
sage: Graph([(1,2),(3,4)])._backend <type 'sage.graphs.base.sparse_graph.SparseGraphBackend'> sage: Graph([(1,2),(3,4)],implementation="networkx")._backend <class 'sage.graphs.base.graph_backends.NetworkXGraphBackend'>
The 'networkx' backend only means that inside of the graph class, at a level you are not supposed to reach except when you write lowlevel graph functions, the graph is stored as a netowrkx object.
That changes nothing to the fact that networkx is still available in Sage. You can import it, call it, and still get a networkx copy of your graphs with Graph.networkx_graph()
. And Sage itself still uses networkx in many places: the stupidest example of that is graphs.CompleteGraph
. Believe it or not, in order to build a complete graph Sage calls networkx' function networkx.complete_graph(n)
.
Nathann
comment:9 in reply to: ↑ 6 Changed 4 years ago by
Should I test something else to ensure that this patch is working well? In particular, how can I test the pickle stuff.
It gets tested automatically. It is done in sage.structure.sage_object
. You can also do it manually with:
sage: sage.structure.sage_object.unpickle_all()
Nathann
comment:10 followup: ↓ 11 Changed 4 years ago by
 Reviewers set to David Coudert
 Status changed from needs_review to needs_work
I did a ./sage t long src/sage/ and all tests pass.
I also tried
sage: sage.structure.sage_object.unpickle_all() /Users/dcoudert/sage/local/lib/python2.7/sitepackages/IPython/core/interactiveshell.py:3035: DeprecationWarning: You unpickled an object which relies on an old data structure. Save it again to update it, for it may break in the future. See http://trac.sagemath.org/18375 for details. exec(code_obj, self.user_global_ns, self.user_ns) /Users/dcoudert/sage/src/bin/sageipython:1: DeprecationWarning: You unpickled an object which relies on an old data structure. Save it again to update it, for it may break in the future. See http://trac.sagemath.org/1000 for details. #!/usr/bin/env python /Users/dcoudert/sage/src/bin/sageipython:1: DeprecationWarning: OrderedAlphabet is deprecated; use Alphabet instead. See http://trac.sagemath.org/8920 for details. #!/usr/bin/env python /Users/dcoudert/sage/src/bin/sageipython:1: DeprecationWarning: gen_py.pari is deprecated, use sage.libs.pari.all.pari instead See http://trac.sagemath.org/17451 for details. #!/usr/bin/env python /Users/dcoudert/sage/local/lib/python2.7/sitepackages/sage/rings/finite_rings/constructor.py:632: DeprecationWarning: The "pari_mod" finite field implementation is deprecated See http://trac.sagemath.org/17297 for details. K = FiniteField_ext_pari(order, name, modulus) /Users/dcoudert/sage/src/bin/sageipython:1: DeprecationWarning: MatrixSpace_ZZ_2x2 is deprecated. Please use MatrixSpace(ZZ,2) instead See http://trac.sagemath.org/17824 for details. #!/usr/bin/env python Successfully unpickled 586 objects. Failed to unpickle 0 objects.
So the patch is working well.
However, in the doc, on page /reference/graphs/sage/graphs/base/overview.html the section The backends
could be updated, at least to mention that the networkx backend is deprecated.
comment:11 in reply to: ↑ 10 Changed 4 years ago by
However, in the doc, on page /reference/graphs/sage/graphs/base/overview.html the section
The backends
could be updated, at least to mention that the networkx backend is deprecated.
Arg. Right.
Nathann
comment:12 Changed 4 years ago by
 Commit changed from 26c47897f8b8312baf2878f323e9d7fe5541c98a to 98c86a41330a3cffacbedc1743467db9aa08117b
comment:13 Changed 4 years ago by
 Status changed from needs_work to needs_review
I updated the doc, which does not mention the netowrkx backend anymore. Note that it is not "deprecated", but totally dropped. Users will still see a deprecation warning for a year, but in the backscene the CGraph backend is the one that will be used.
Nathann
comment:14 Changed 4 years ago by
 Status changed from needs_review to positive_review
For me the patch is now good to go.
comment:15 Changed 4 years ago by
Thanks !
comment:16 in reply to: ↑ 8 Changed 4 years ago by
This ticket removes a feature you probably did not know even existed: The 'networkx' backend only means that inside of the graph class, at a level you are not supposed to reach except when you write lowlevel graph functions, the graph is stored as a netowrkx object.
Okay, I am fine with that  actually I did know that feature existed.
Believe it or not, in order to build a complete graph Sage calls networkx' function
networkx.complete_graph(n)
.
Yes, in fact I think there are other places where that occurs :) Thanks for the clarification!
comment:17 Changed 4 years ago by
 Branch changed from public/18375 to 98c86a41330a3cffacbedc1743467db9aa08117b
 Resolution set to fixed
 Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. New commits:
trac #18375: Drop the NetworkX graph backend