replace .copy() with .__copy__() in graphs/graph.py

[aghitza]

See also #111, where this originates.

CC: @sagetrac-sage-combinat @williamstein @sagetrac-mvngu @aghitza

Component: graph theory

Author: Karl-Dieter Crisman

Reviewer: Nathann Cohen, Robert Miller

Merged: sage-4.3.rc1

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

[aghitza]

comment:1

Note that this has consequences for several files in combinat/.

[kcrisman]

comment:2

Note that we will likely need deprecation warnings. See discussion at #6521.

[kcrisman]

comment:3

There is one small problem with this. Doing the naive change -

    def __copy__(self, implementation='networkx', sparse=None):

yields:

sage: g=Graph({0:[0,1,1,2]})
sage: copy(g)
Looped multi-graph on 3 vertices
sage: g.__copy__(sparse=True)
Looped multi-graph on 3 vertices
sage: copy(g,sparse=True)
---------------------------------------------------------------------------
TypeError: copy() got an unexpected keyword argument 'sparse'

It's not clear to me how to deal with this; changing the global 'copy' to handle keywords seems ill-advised. On the other hand, there definitely is code (elsewhere) that uses the keywords implementation and sparse, at least in graph_generators.py.

[kcrisman]

Author: Karl-Dieter Crisman

[kcrisman]

comment:4

I resolved this as best I could. Attached patch should catch all remaining instances of .copy() that don't belong to Python objects that require it (i.e. dicts have only copy, not 'copy'!)

[kcrisman]

Based on 4.2.1

[nathanncohen]

comment:5

Attachment: trac_6522.patch.gz

Hello !!! Could you use the new methods defined in #7515 for the functions you deprecate ? It would ease the work in #7559 :-)

[rlmill]

comment:6

I can't say that I agree with the point of this ticket.

Certainly there should be a __copy__ defined for graphs, so that

sage: G = copy(Graph())

works. However, the main use case of the copy method for graphs (for me, at least) is when I want to change underlying implementations. What was

sage: G = graphs.PetersenGraph()
sage: C = G.copy(implementation='c_graph', sparse=False)

won't work as

sage: G = graphs.PetersenGraph()
sage: copy(G, implementation='c_graph', sparse=False)

but instead we now need to do:

sage: G = graphs.PetersenGraph()
sage: C = G.__copy__(implementation='c_graph', sparse=False)

Which is an ugly, pointless change in API. Why don't we just define __copy__, and acknowledge that in some cases, it makes sense for objects to have a copy method?

[kcrisman]

comment:7

I think that is fine (despite the time it took to do this), because that point makes tons of sense! But perhaps the people who originated this idea in #111 should weigh in before we just add a __copy__ and don't remove copy - I am cc:ing a few of them.

[nathanncohen]

comment:8

Ok, let's forget about #7515 and #7559 for this patch, if you are short on time it is not worth making this patch wait :-)

Besides, taking care of #7559 will be a huge work, with of without 10 modifications more !

Nathann

[rlmill]

comment:9

Changing the component to graph theory so I can track this:

see http://groups.google.com/group/sage-devel/browse_thread/thread/70aacbd1dcc83497

[kcrisman]

Based on 4.3.alpha1

[kcrisman]

comment:10

Attachment: trac_6522-final.patch.gz

Okay, here is how I dealt with these issues.

1. We can't use the generic deprecation thing from Improved deprecation and renaming of function and methods #7515 here, because it would say to use copy instead of copy! Unfortunately. On the plus side, that's one less for replace all the deprecation warning using deprecated_function_alias whenever possible #7559.

2. I have not deprecated copy() from the generic graph class, only the yang-baxter one. There is now a __copy__ for generic graphs. In order to deal with a tricky thing on Dynkin diagrams, I had to define a __copy__ method for them, which however is EXACTLY the same as the generic Python copy from the copy module.

I really, really worked hard to make sure I caught every possible place where this causes problems, and it passes all doctests, but please think hard where it would make a difference. I also hope I won't have to rebase it again :)

[rlmill]

Reviewer: Nathann Cohen, Robert Miller

[rlmill]

comment:11

Ran tests in sage/graphs and sage/combinat. Looks good to me (I think some of those imports are unnecessary, but not a showstopper)

[mwhansen]

Merged: sage-4.3.rc1