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

deprecate parameter copy in networkx_graph #27491

Closed
rajat1433 mannequin opened this issue Mar 15, 2019 · 23 comments
Closed

deprecate parameter copy in networkx_graph #27491

rajat1433 mannequin opened this issue Mar 15, 2019 · 23 comments

Comments

@rajat1433
Copy link
Mannequin

rajat1433 mannequin commented Mar 15, 2019

Done in this ticket:

CC: @dcoudert

Component: graph theory

Author: Rajat Mittal

Branch/Commit: cefb5fb

Reviewer: David Coudert

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

@rajat1433 rajat1433 mannequin added this to the sage-8.7 milestone Mar 15, 2019
@rajat1433
Copy link
Mannequin Author

rajat1433 mannequin commented Mar 15, 2019

Branch: u/gh-rajat1433/27491_networkx_graph

@rajat1433 rajat1433 mannequin self-assigned this Mar 15, 2019
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 15, 2019

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

1228fbabug fix

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 15, 2019

Commit: 1228fba

@rajat1433
Copy link
Mannequin Author

rajat1433 mannequin commented Mar 15, 2019

Author: Rajat Mittal

@rajat1433
Copy link
Mannequin Author

rajat1433 mannequin commented Mar 15, 2019

Reviewer: dcoudert

@rajat1433 rajat1433 mannequin added the s: needs review label Mar 15, 2019
@dcoudert
Copy link
Contributor

comment:4

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 15, 2019

Changed commit from 1228fba to 3b48ef2

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 15, 2019

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

3b48ef2copy removed

@rajat1433
Copy link
Mannequin Author

rajat1433 mannequin commented Mar 15, 2019

New commits:

3b48ef2copy removed

@rajat1433

This comment has been minimized.

@rajat1433 rajat1433 mannequin changed the title bug fix: default value true instead of false in networkx_graph method bug fix: removal of copy parameter from networkx_graph Mar 15, 2019
@rajat1433

This comment has been minimized.

@rajat1433 rajat1433 mannequin added p: major / 3 and removed p: minor / 4 labels Mar 15, 2019
@rajat1433
Copy link
Mannequin Author

rajat1433 mannequin commented Mar 15, 2019

comment:8

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.

@dcoudert
Copy link
Contributor

comment:9

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.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 15, 2019

Changed commit from 3b48ef2 to ba0855a

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 15, 2019

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

ba0855aremoved unnecessary code

@rajat1433
Copy link
Mannequin Author

rajat1433 mannequin commented Mar 16, 2019

Changed reviewer from dcoudert to David Coudert

@dcoudert
Copy link
Contributor

comment:12

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.

@dcoudert

This comment has been minimized.

@dcoudert dcoudert changed the title bug fix: removal of copy parameter from networkx_graph deprecate parameter copy in networkx_graph Mar 17, 2019
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 17, 2019

Changed commit from ba0855a to cefb5fb

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 17, 2019

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

cefb5fbdeprecated copy

@rajat1433
Copy link
Mannequin Author

rajat1433 mannequin commented Mar 17, 2019

comment:14

deprecated the copy parameter.

@dcoudert
Copy link
Contributor

comment:15

LGTM

@rajat1433 rajat1433 mannequin modified the milestones: sage-8.7, sage-8.8 Mar 24, 2019
@vbraun
Copy link
Member

vbraun commented Mar 25, 2019

Changed branch from u/gh-rajat1433/27491_networkx_graph to cefb5fb

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