Meta-ticket: Support Python's __copy__ / __deepcopy__ protocol

[cnassau]

Most Sage objects are immutable. Nevertheless, copy and deepcopy make copies (through pickling/unpickling) for them because we have not provided the classes with __copy__ methods (which should just return the object) and __deepcopy__ methods (which in many cases should just return the object).

sage: a = 0
sage: copy(a) is a
False

Another symptom (from the original ticket description): Copying of (immutable!) LazyFamily objects only works for families that can be pickled:

   sage: def unpicklableFamily():
   ...       x = 10
   ...       return LazyFamily([1,2,3], lambda n: x*n)
   sage: f = unpicklableFamily() 
   sage: copy(f)
   Traceback (most recent call last):
   ...
   ValueError: Cannot pickle code objects from closures

The case of __deepcopy__ of immutable container-like objects (like Family or Sequence) is a bit trickier: Is immutability intended to imply that the elements are immutable too? In this ticket, we make no such assumption.

Tickets:

• trivial __copy__ and __deepcopy__ methods for number types #32454 trivial __copy__ and __deepcopy__ methods for number types
• trivial __copy__ and __deepcopy__ methods for number field elements #32478 trivial __copy__ and __deepcopy__ methods for number field elements
• trivial __copy__ and __deepcopy__ methods for symbolic expressions and functions #32480 trivial __copy__ and __deepcopy__ methods for symbolic expressions and functions
• trivial __copy__ and __deepcopy__ methods for Family and Set #32462 trivial __copy__ and __deepcopy__ methods for Family and Set
• __copy__ methods for all classes that define "copy" methods (outside of sage.tensor, sage.manifolds) #32453 __copy__ methods for all classes that define copy methods
• sage.tensor, sage.manifolds: __copy__, __deepcopy__ methods for all classes that define "copy" methods #32476 sage.tensor, sage.manifolds: __copy__, __deepcopy__ methods for all classes that define copy methods
• copy(CombinatorialFreeModule.Element) broken by #22632 #23075 copy(CombinatorialFreeModule.Element) broken by Cythonize CombinatorialFreeModuleElement #22632
• Fix some more deepcopy/caching issues in the quadratic forms code (followup to #5403) #5417 Fix some more deepcopy/caching issues in the quadratic forms code
  Related:
• sage.structure.mutability: Add global is_mutable, is_immutable functions #30387

Depends on #32454

CC: @tscrim @mjungmath @nbruin @kwankyu @williamstein @mwageringel @kcrisman

Component: pickling

Keywords: LazyFamily, copy, pickle

Author: Matthias Koeppe

Branch/Commit: u/mkoeppe/__copy___and___deepcopy___methods_for_all_immutable_sage_objects @ 2bd01ae

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

[cnassau]

Author: cnassau

[cnassau]

Changed author from cnassau to Christian Nassau

[nthiery]

comment:4

Hi Christian!

Families are (semantically) immutable objects. Why would we want to copy them?

Cheers,
Nicolas

[cnassau]

comment:5

Replying to @nthiery:

Hi Christian!

Families are (semantically) immutable objects. Why would we want to copy them?

Good point... I stumbled across this issue while I was struggling to create a disjoint union of a dynamically generated family of LazyFamily objects, while the real problem was that I was using closures with reference to a loop variable. I eventually resolved this issue, and my solution does not requiry any copies now. So I agree that there's no need to create shallow copies.

I do think that user code can expect that copy(any_sage_object) does not throw an error. Maybe LazyFamily.__copy__(self) should just return self? This would be in accordance with

sage: copy(Integers()) is Integers()
True

Cheers,
Christian

[nthiery]

comment:6

Replying to @cnassau:

I do think that user code can expect that copy(any_sage_object) does not throw an error. Maybe LazyFamily.__copy__(self) should just return self? This would be in accordance with

sage: copy(Integers()) is Integers()
True

Yes, it would make sense to have copy(x) return x for all immutable objects in Sage. I am not sure how to achieve this though: we do not (yet?) have a class for providing code for all immutable objects in Sage, and I would not want to force every relevant class to reimplement a trivial __copy__ method.

For the case at hand (families), maybe one could add a method Parent.copy returning self? As far as I can tell, parents are required to be immutable anyway. But one should double check this.

Cheers,
Nicolas

[cnassau]

A patch for this problem

[jdemeyer]

comment:7

Attachment: lazyfamcopy.patch.gz

[videlec]

comment:11

Hi,

As Nicolas suggested in his comnent 6, __copy__ should be implemented as

def __copy__(self):
    return self

It is standard Python for immutable objects

sage: t = (1,2,3)
sage: copy(t) is t
True
sage: i = 1231491283r
sage: copy(i) is i
True 

Vincent

[mkoeppe]

Changed author from Christian Nassau to none

[mkoeppe]

comment:48

For the pattern in comment:45, with #29101 ("Refined protocol for _element_constructor_ ") we would generally be able to replace copy(ray) by ray.parent()(ray, mutable=True, copy=True); but perhaps we should define a shorthand for that.

• Guide for parents with immutable elements #32353 proposes new methods mutable, immutable for this
• src/sage/combinat/constellation.py defines .mutable_copy
• src/sage/graphs/generic_graph.py defines GenericGraph.copy, which has a parameter immutable=None:
• some classes have copy methods with optional arguments that could be extended (see also __copy__ methods for all classes that define "copy" methods (outside of sage.tensor, sage.manifolds) #32453) to allow ray.copy(mutable=True) (proposed in https://groups.google.com/g/sage-devel/c/DNrbtItMVmQ/m/BbMwM-qeAQAJ)

Decision time...

[mkoeppe]

comment:49

Replying to @mkoeppe:

There are some doctest failures now from bad code like this in src/sage/geometry/cone.py:

                    # rays has immutable elements
                    rays = [copy(ray) for ray in rays]

                    for i, ray in enumerate(rays):
                        rays[i][0] = pm * (ray[0].abs() + 1)

This pattern -- expecting that ray.__copy__ (and thus copy(ray)) make a mutable copy of an immutable object -- appears to come from a decision made many many years ago in Sage (#111, #6522, #15132, ...). This was probably a good idea at the time, but I think it is not compatible with current Python practices.

[mkoeppe]

Dependencies: #32454

[kcrisman]

comment:52

This pattern -- expecting that ray.__copy__ (and thus copy(ray)) make a mutable copy of an immutable object -- appears to come from a decision made many many years ago in Sage (#111, #6522, #15132, ...). This was probably a good idea at the time, but I think it is not compatible with current Python practices.

That may indeed be the case. As you say, at the time the goal was standardized Sage command structure, but if there is a more (modern) Pythonic way to handle this, #6522 alone shows how much gnashing of teeth surrounded some of these changes at the time, so more power to you as long as any syntax changes would be documented.

One should also be sure that things which would "naturally" be seen as copyable are, but other than Sequence which I am not as familiar with, it seems like e.g. vectors can still be copied unless asked not to be, so seems fine to me.

[mkoeppe]

comment:53

Regarding the pair of methods v.mutable(), v.immutable() proposed in #32353: v.immutable() is clear, of course, and shorter than v.copy(mutable=False). But I think it is unclear what v.mutable() is supposed to be if v is already mutable. Does it return a copy? Then the method's name should say so that it is obvious to users / readers of the code that mutating the result is not only allowed but also safe.
So a pair v.mutable_copy(), v.immutable_copy() would be fine, although I'm not sure if it's much better than v.copy(mutable=True), v.copy(mutable=False)

[mkoeppe]

comment:57

Some more data points on other libraries:

• numpy's ndarrays (and subclasses) have a copy method with an optional parameter that controls the memory layout of the copy; the __copy__ method is equivalent to one of these choices.
• sympy defines copy methods but no deepcopy, __copy__, or __deepcopy__ methods