copy(CombinatorialFreeModule.Element) broken by #22632

[cnassau]

The following problem showed up with 8.0.beta8: the copy of an IndexedFreeModuleElement seems to be not functional:

sage: F=CombinatorialFreeModule(ZZ,ZZ)
sage: a=F.an_element() ; a
3*B[-1] + B[0] + 3*B[1]
sage: copy(a)
<repr(<sage.combinat.free_module.CombinatorialFreeModule_with_category.element_class at 0x7f5d735e7808>) failed: AttributeError: 'NoneType' object has no attribute 'items'>
sage: type(copy(a)._monomial_coefficients)
<type 'NoneType'>

Part of Meta-ticket #13811: Support Python's __copy__ / __deepcopy__ protocol

CC: @nthiery @jdemeyer @fchapoton @tscrim @mjungmath

Component: combinatorics

Author: Travis Scrimshaw

Branch/Commit: ac1bf2c

Reviewer: Matthias Koeppe

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

[nthiery]

comment:1

Thanks for the report.

Hmm, that's due to the fact that _monomial_coefficients is a Cython attribute now, so not automatically copied. So we need a custom copy method. We have to decide whether we want copy(x) to just return x (since it's supposed to be immutable) or really make a copy.

What's the use case where this showed up?

[cnassau]

comment:2

Replying to @nthiery:

What's the use case where this showed up?

I was reparenting an element from one module to a slightly altered clone of its parent (so I was probably violating the immutability assumption regarding elements, of which I was unaware). My code was similar to this:

def corresponding_element_in_clone(parent,elem):
   N = parent.create_modified_clone(parent)
   x = copy(elem)
   x._set_parent(N)
   return x

Using "deepcopy" seems to still work.

[tscrim]

comment:3

Well, I would say yes and no. By copying the object, you're going around the immutability by making a temporarily mutable object. This isn't quite your question, but if you wanted to (safely) manipulate the underlying dict, you could get get a copy by calling d = x.monomial_coefficients(), mutating that, and then F._from_dict(d) (with appropriate options, probably coerce=False and remove_zeros=False). Yet, you're not changing the "defining" information of the object, just what parent it is associated to. So you could do x.monomial_coefficients(copy=False) to get the underlying dict and then do the F._from_dict(d). In the abstract, this should be just as fast, and is the (IMO) correct way to do it.

However, the fact that copy fails is not good. I would say copy(x) is x should be True in this case because it is only an immutable object.

[nthiery]

comment:4

Thanks for the feedback. I agree with Travis. Let's go for an idempotent copy operator. Do you see a natural place to advertise that the elements are immutable?

[tscrim]

comment:5

For the most part, elements in Sage are considered immutable so we can do hashing, but I guess this conflicts with the rest of the linear algebra in Sage. So I would put a small note/example in the IndexedFreeModuleElement class doc.

[mkoeppe]

comment:7

Setting new milestone based on a cursory review of ticket status, priority, and last modification date.

[mkoeppe]

comment:11

Still broken in 9.7.beta8

[sheerluck]

comment:12

--- a/src/sage/combinat/free_module.py
+++ b/src/sage/combinat/free_module.py
@@ -358,10 +358,12 @@
             sage: A.__class__.element_class.__module__
             'sage.combinat.free_module'
         """
-        return self.__make_element_class__(self.Element,
+        cls = self.__make_element_class__(self.Element,
                                            name="%s.element_class" % self.__class__.__name__,
                                            module=self.__class__.__module__,
                                            inherit=True)
+        setattr(cls, '__copy__', lambda x: x)
+        return cls

     def __init__(self, R, basis_keys=None, element_class=None, category=None,
                  prefix=None, names=None, **kwds):

[tscrim]

comment:13

Very strong -1 on the comment:12 proposal. It is a hack and only applies to CFM elements.

I think it should be sufficient to do

def __copy__(self):
    return self
def __deepcopy__(self):
    return self

in IndexedFreeModuleElement. Although perhaps the __deepcopy__ can actually copy the dictionary (to be more in line with copy vs deepcopy).

[mkoeppe]

comment:14

Are CFM elements immutable?

[tscrim]

comment:15

Yes, like most (all?) other elements in Sage (other than matrices, vectors, and (di)graphs).

[mkoeppe]

comment:16

Then __deepcopy__ should not copy anything either

[tscrim]

comment:18

Here are the methods. I am now leaning towards not really saying anything because it doesn't implement any of the other mutability operations than vector has. This would also be consistent with the linear morphisms code too.

---

New commits:

958359f

Making the copy of IndexedFreeModuleElement idempotent since it is immutable.

[tscrim]

Branch: public/linear_algebra/indexed_elt_copy-23075

[tscrim]

Commit: 958359f

[tscrim]

Author: Travis Scrimshaw

[mkoeppe]

comment:19

Looking good, with the nitpick that "copy" is not only idempotent here but actually the identity.

[sagetrac-git]

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

ac1bf2c

Tweak to module docstring of indexed_element.pyx.

[sagetrac-git]

Changed commit from 958359f to ac1bf2c

[tscrim]

comment:21

Replying to @mkoeppe:

Looking good, with the nitpick that "copy" is not only idempotent here but actually the identity.

Fair enough. I have changed it.

[mkoeppe]

comment:22

just waiting for a green bot

[mkoeppe]

Reviewer: Matthias Koeppe

[tscrim]

comment:24

Thank you.

[vbraun]

Changed branch from public/linear_algebra/indexed_elt_copy-23075 to ac1bf2c