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: sage-6.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 non-deprecated data structure (sparse graph, by default).
  • Two random_stress functions are removed, as they work by comparing the behaviour of c_graph and networkx 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 ncohen

  • Branch set to public/18375
  • Cc dcoudert borassi vdelecroix added
  • Status changed from new to needs_review

comment:2 Changed 4 years ago by git

  • Commit set to d5c870a03e851e8b2bb4b9defd77083cc6847a91

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

d5c870atrac #18375: Drop the NetworkX graph backend

comment:3 Changed 4 years ago by git

  • Commit changed from d5c870a03e851e8b2bb4b9defd77083cc6847a91 to 26c47897f8b8312baf2878f323e9d7fe5541c98a

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

26c4789trac #18375: Drop the NetworkX graph backend

comment:4 follow-up: Changed 4 years ago by vdelecroix

You do not like these random_stress functions?

comment:5 in reply to: ↑ 4 Changed 4 years ago by ncohen

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 follow-up: Changed 4 years ago by dcoudert

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 follow-up: Changed 4 years ago by kcrisman

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 ; follow-up: Changed 4 years ago by ncohen

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 low-level 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 ncohen

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

Last edited 4 years ago by ncohen (previous) (diff)

comment:10 follow-up: Changed 4 years ago by dcoudert

  • 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/site-packages/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/sage-ipython: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/sage-ipython: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/sage-ipython: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/site-packages/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/sage-ipython: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 ncohen

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 git

  • Commit changed from 26c47897f8b8312baf2878f323e9d7fe5541c98a to 98c86a41330a3cffacbedc1743467db9aa08117b

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

8f14d9dtrac #18375: Merged with 6.8.beta0
98c86a4trac #18375: Update the doc

comment:13 Changed 4 years ago by ncohen

  • 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 dcoudert

  • Status changed from needs_review to positive_review

For me the patch is now good to go.

comment:15 Changed 4 years ago by ncohen

Thanks !

comment:16 in reply to: ↑ 8 Changed 4 years ago by kcrisman

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 low-level 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 vbraun

  • Branch changed from public/18375 to 98c86a41330a3cffacbedc1743467db9aa08117b
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.