#27491 closed defect (fixed)

deprecate parameter copy in networkx_graph

Reported by: gh-rajat1433 Owned by: gh-rajat1433
Priority: major Milestone: sage-8.8
Component: graph theory Keywords:
Cc: dcoudert Merged in:
Authors: Rajat Mittal Reviewers: David Coudert
Report Upstream: N/A Work issues:
Branch: cefb5fb (Commits) Commit: cefb5fb2659f7d5d7d9f21a1306e7703d76cbd75
Dependencies: Stopgaps:

Description (last modified by dcoudert)

Done in this ticket:

  • deprecate parameter copy of method networkx_graph. This parameter was useful for the _nxg backend that we don't have anymore.
  • remove remaining pieces of calls to ._backend._nxg
  • remove deprecated class NetworkXGraphDeprecated. It should have been removed in #27166, but has been forgotten.

Change History (17)

comment:1 Changed 11 months ago by gh-rajat1433

  • Branch set to u/gh-rajat1433/27491_networkx_graph
  • Owner changed from (none) to gh-rajat1433

comment:2 Changed 11 months ago by git

  • Commit set to 1228fba5d93ba90f309e0f62e25ef96ff641d4e3

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

1228fbabug fix

comment:3 Changed 11 months ago by gh-rajat1433

  • Authors set to Rajat Mittal
  • Reviewers set to dcoudert
  • Status changed from new to needs_review

comment:4 Changed 11 months ago by dcoudert

This is a very good catch. However, what you propose to do is not what should be done.

In #27166, we removed deprecated networkx backends, but we forgot to update other parts.

  • In file src/sage/graphs/base/graph_backends.pyx: remove class NetworkXGraphDeprecated
  • in method networkx_graph: remove parameter copy and update the code so that the method always returns a networkx graph.
  • the name of this ticket will have to be updated accordingly

comment:5 Changed 11 months ago by git

  • Commit changed from 1228fba5d93ba90f309e0f62e25ef96ff641d4e3 to 3b48ef20b8e5c1556c787fec42156a19776bd24a

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

3b48ef2copy removed

comment:6 Changed 11 months ago by gh-rajat1433

  • Description modified (diff)
  • Summary changed from bug fix: default value true instead of false in networkx_graph method to bug fix: removal of copy parameter from networkx_graph

New commits:

3b48ef2copy removed

comment:7 Changed 11 months ago by gh-rajat1433

  • Description modified (diff)
  • Priority changed from minor to major

comment:8 Changed 11 months ago by gh-rajat1433

I am not able to locate _backend._nxg in any backend file in the networkx_graph method

        try:
            return self._backend._nxg
        except Exception:

can you point the location or does it always throw an error , if so we can remove it.

Last edited 11 months ago by gh-rajat1433 (previous) (diff)

comment:9 Changed 11 months ago by dcoudert

We don't have this backend anymore. This is a remaining of the very first releases of Sagemath that we have not fully removed yet (I forgot to do it in #27166).

You can also remove this from trees.pyx

#        from networkx import MultiGraph
#        G = Graph(self.vertices)
#        cdef object XG = G._backend._nxg
#
#        for i from 2 <= i <= self.vertices:
#            vertex1 = i - 1
#            vertex2 = self.current_level_sequence[i - 1] - 1
#            XG.add_edge(vertex1, vertex2)
#
#        return G

        # Currently, c_graph does not have all the same functionality as networkx.
        # Until it does, we can't generate graphs using the c_graph backend even
        # though it is twice as fast (for our purposes) as networkx.

comment:10 Changed 11 months ago by git

  • Commit changed from 3b48ef20b8e5c1556c787fec42156a19776bd24a to ba0855ab0eb0f4629312033299c9697aa8998114

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

ba0855aremoved unnecessary code

comment:11 Changed 11 months ago by gh-rajat1433

  • Reviewers changed from dcoudert to David Coudert

comment:12 Changed 10 months ago by dcoudert

  • Description modified (diff)
  • Summary changed from bug fix: removal of copy parameter from networkx_graph to deprecate parameter copy in networkx_graph

I was a little to fast. To be clean, we should add a deprecation warning for parameter copy.

Could you:

  • put back parameter copy as input parameter, but without documentation. So
    -    def networkx_graph(self):
    +    def networkx_graph(self, copy=True):
    
  • and add the following at the very beginning of the method
    +        if copy is not True:
    +            deprecation(27491, "parameter copy is removed")
    

Then, in one year, we will completely remove this parameter.

Another small modification:

-        N.add_nodes_from(self.vertices())
+        N.add_nodes_from(self)

Method vertices sorts vertices by default, and returns a list of vertices. Method add_nodes_from can take an iterator as input. So we save the creation of the list.

I changed the description of this ticket to be consistent with what it does.

comment:13 Changed 10 months ago by git

  • Commit changed from ba0855ab0eb0f4629312033299c9697aa8998114 to cefb5fb2659f7d5d7d9f21a1306e7703d76cbd75

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

cefb5fbdeprecated copy

comment:14 Changed 10 months ago by gh-rajat1433

deprecated the copy parameter.

comment:15 Changed 10 months ago by dcoudert

  • Status changed from needs_review to positive_review

LGTM

comment:16 Changed 10 months ago by gh-rajat1433

  • Milestone changed from sage-8.7 to sage-8.8

comment:17 Changed 10 months ago by vbraun

  • Branch changed from u/gh-rajat1433/27491_networkx_graph to cefb5fb2659f7d5d7d9f21a1306e7703d76cbd75
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.