Meta-ticket: Immutability for manifold objects

[mjungmath]

Tensor fields, scalar fields, sections and (bundle) connections are a priori mutable. However, according to #30181, it should be possible to make them immutable and in conclusion to make them hashable.

Connections and bundle connections are already hashable even though they are not immutable. This will be changed accordingly in this ticket.

• Scalar Fields: Immutability for scalar fields #30266
• Scalar Fields II: Immutability of Scalar Fields II #30311
• Tensor Fields (without hash): Immutability of Tensor Fields #30274, Follow Up: Mutability of Sections and Tensor Fields #31255
• Affine Connections: Immutability of Affine Connections #30280
• Bundle Connections: Immutability of Bundle Connections #30284
• Sections (without hash): Immutability for Sections #30288, Follow Up: Mutability of Sections and Tensor Fields #31255
• Chart Functions (without hash): Immutability of chart functions #30310
• Mixed Differential Forms: Immutability for mixed forms #31706

Depends on #30181
Depends on #30266
Depends on #30274
Depends on #30280
Depends on #30284
Depends on #30288
Depends on #30310
Depends on #30311
Depends on #31706

CC: @egourgoulhon @mkoeppe @tscrim

Component: manifolds

Keywords: immutability

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

[mjungmath]

Dependencies: #30181

[mjungmath]

comment:6

I haven't found any active use of the Mutability class in the whole Sage library. For the sake of generality, it would be good to have a class which inherits from SageObject and represents a mutable element. This would be good for e.g. connections. Either one adds such a class to sage.structure.mutability or, since the aforementioned class is not actively used anywhere, rename it accordingly (like ElementWithMutability) and adding an inheritance from SageObject. Of course, I would open another ticket for that. Probably, the ModuleElementWithMutability should be shifted in that file, too?

There is another issue, I have encountered due to the dependence of #30181. I simply changed the following lines in sage.manifolds.differentiable.tensorfield:

-from sage.structure.element import ModuleElement
+from sage.structure.element import ModuleElementWithMutability

-class TensorField(ModuleElement):
+class TensorField(ModuleElementWithMutability):

-        ModuleElement.__init__(self, parent)
+        super().__init__(parent)

and I get the following error message:

sage: M = Manifold(2, 'M')
sage: U = M.open_subset('U'); V = M.open_subset('V')
sage: c_xy.<x,y> = U.chart(); c_uv.<u,v> = V.chart()
Traceback (most recent call last)
...
TypeError: __init__() missing 1 required positional argument: 'tensor_type'

[mkoeppe]

comment:7

If you super, all __init__s need to have a compatible signature...

[tscrim]

comment:8

Replying to @mjungmath:

For the sake of generality, it would be good to have a class which inherits from SageObject and represents a mutable element. This would be good for e.g. connections.

Strong -1 This is a mixin class. Just add it to your inheritance. You don't have any Cython classes, so you can use multiple inheritance without any issues. I don't expect too many cases where having Cython is useful for elements with mutabiliy.

Probably, the ModuleElementWithMutability should be shifted in that file, too?

-1 All of the ABCs for the elements are in element.pyx. Moving it would make it harder to find.

[mjungmath]

comment:9

Replying to @tscrim:

Strong -1 This is a mixin class. Just add it to your inheritance. You don't have any Cython classes, so you can use multiple inheritance without any issues. I don't expect too many cases where having Cython is useful for elements with mutabiliy.

You mean

-class AffineConnection(SageObject):
+class AffineConnection(SageObject, Mutability):

Why is it working in this case? SageObject and Mutability are both Cython classes. I am a little bit confused...sorry.

Probably, the ModuleElementWithMutability should be shifted in that file, too?

-1 All of the ABCs for the elements are in element.pyx. Moving it would make it harder to find.

Alright, that makes sense.

---

The inheritance above seems to cause some trouble, I think due to the __reduce__ method in Mutability:

File "src/sage/manifolds/differentiable/affine_connection.py", line 350, in sage.manifolds.differentiable.affine_connection.AffineConnection.__init__
Failed example:
    TestSuite(nab).run()
Expected nothing
Got:
    Failure in _test_pickling:
    Traceback (most recent call last):
      File "/home/michi/Projekte/sage-devel/local/lib/python3.7/site-packages/sage/misc/sage_unittest.py", line 296, in run
        test_method(tester=tester)
      File "sage/structure/sage_object.pyx", line 639, in sage.structure.sage_object.SageObject._test_pickling (build/cythonized/sage/structure/sage_object.c:4953)
        tester.assertEqual(loads(dumps(self)), self)
      File "/home/michi/Projekte/sage-devel/local/lib/python3.7/unittest/case.py", line 839, in assertEqual
        assertion_func(first, second, msg=msg)
      File "/home/michi/Projekte/sage-devel/local/lib/python3.7/unittest/case.py", line 832, in _baseAssertEqual
        raise self.failureException(msg)
    AssertionError: <sage.structure.mutability.Mutability object at 0x7fc1d06dd410> != Affine connection nabla on the 3-dimensional differentiable manifold M
    ------------------------------------------------------------
    The following tests failed: _test_pickling

[tscrim]

comment:10

Replying to @mjungmath:

Replying to @tscrim:

Strong -1 This is a mixin class. Just add it to your inheritance. You don't have any Cython classes, so you can use multiple inheritance without any issues. I don't expect too many cases where having Cython is useful for elements with mutabiliy.

You mean

-class AffineConnection(SageObject):
+class AffineConnection(SageObject, Mutability):

Why is it working in this case? SageObject and Mutability are both Cython classes. I am a little bit confused...sorry.

No. A Cython class cannot inherit from multiple things, but a Python class (as you have here) can. The classes it inherits from can both be Cython classes.

---

The inheritance above seems to cause some trouble, I think due to the __reduce__ method in Mutability:

File "src/sage/manifolds/differentiable/affine_connection.py", line 350, in sage.manifolds.differentiable.affine_connection.AffineConnection.__init__
Failed example:
    TestSuite(nab).run()
Expected nothing
Got:
    Failure in _test_pickling:
    Traceback (most recent call last):
      File "/home/michi/Projekte/sage-devel/local/lib/python3.7/site-packages/sage/misc/sage_unittest.py", line 296, in run
        test_method(tester=tester)
      File "sage/structure/sage_object.pyx", line 639, in sage.structure.sage_object.SageObject._test_pickling (build/cythonized/sage/structure/sage_object.c:4953)
        tester.assertEqual(loads(dumps(self)), self)
      File "/home/michi/Projekte/sage-devel/local/lib/python3.7/unittest/case.py", line 839, in assertEqual
        assertion_func(first, second, msg=msg)
      File "/home/michi/Projekte/sage-devel/local/lib/python3.7/unittest/case.py", line 832, in _baseAssertEqual
        raise self.failureException(msg)
    AssertionError: <sage.structure.mutability.Mutability object at 0x7fc1d06dd410> != Affine connection nabla on the 3-dimensional differentiable manifold M
    ------------------------------------------------------------
    The following tests failed: _test_pickling

Yes, but not because it does not inherit from SageObject. It is purely an implementation issue trying to keep immutable objects immutable under pickling. I might argue that the Mutability should not have a __reduce__. Although some part of the issue is that you do not handle pickling in AffineConnection.

[mjungmath]

comment:11

Replying to @tscrim:

Why is it working in this case? SageObject and Mutability are both Cython classes. I am a little bit confused...sorry.

No. A Cython class cannot inherit from multiple things, but a Python class (as you have here) can. The classes it inherits from can both be Cython classes.

I'm confused because SageObject and Mutability both add methods and

-class FreeModuleTensor(ModuleElement):
+class FreeModuleTensor(ModuleElement, Mutability):

won't work. What is the difference? Is it because ModuleElement already is inherited?

Yes, but not because it does not inherit from SageObject. It is purely an implementation issue trying to keep immutable objects immutable under pickling. I might argue that the Mutability should not have a __reduce__. Although some part of the issue is that you do not handle pickling in AffineConnection.

Mh. Should I then overload __reduce__ or add set_immutable() etc. manually? Unfortunately, I don't know much about pickling.

[mjungmath]

Changed dependencies from #30181 to #30181, #30266

[mjungmath]

comment:14

Let's work step-wise here. Scalar fields made no problem, I have created a new ticket devoted to them: #30266.

[mjungmath]

Changed dependencies from #30181, #30266 to #30181, #30266, #30274

[tscrim]

comment:18

Replying to @mjungmath:

Replying to @tscrim:

Why is it working in this case? SageObject and Mutability are both Cython classes. I am a little bit confused...sorry.

No. A Cython class cannot inherit from multiple things, but a Python class (as you have here) can. The classes it inherits from can both be Cython classes.

I'm confused because SageObject and Mutability both add methods and

-class FreeModuleTensor(ModuleElement):
+class FreeModuleTensor(ModuleElement, Mutability):

won't work. What is the difference? Is it because ModuleElement already is inherited?

Won't work how? The difference with what? The pickling/__reduce__ issue is because of the implementation of Mutability, not a Python/Cython issue. I need something more precise here.

Yes, but not because it does not inherit from SageObject. It is purely an implementation issue trying to keep immutable objects immutable under pickling. I might argue that the Mutability should not have a __reduce__. Although some part of the issue is that you do not handle pickling in AffineConnection.

Mh. Should I then overload __reduce__ or add set_immutable() etc. manually? Unfortunately, I don't know much about pickling.

You should overload __reduce__. Although I am not sure I agree with the Mutability having a __reduce__ method (or at least once like that; seems counterproductive).

[mjungmath]

comment:19

Replying to @tscrim:

You should overload __reduce__. Although I am not sure I agree with the Mutability having a __reduce__ method (or at least once like that; seems counterproductive).

Would you agree if I remove __reduce__ from Mutability?

Won't work how? The difference with what?

My example above raises a type error while doctesting:

TypeError: multiple bases have instance lay-out conflict

Wasn't that the original intention to write a dedicated ModuleElementWithMutability class? However, due to your argument, at least if I understand it correctly, this should work.

[tscrim]

comment:20

Replying to @mjungmath:

Replying to @tscrim:

You should overload __reduce__. Although I am not sure I agree with the Mutability having a __reduce__ method (or at least once like that; seems counterproductive).

Would you agree if I remove __reduce__ from Mutability?

Yes, but that should be done on a separate ticket.

Won't work how? The difference with what?

My example above raises a type error while doctesting:

TypeError: multiple bases have instance lay-out conflict

Wasn't that the original intention to write a dedicated ModuleElementWithMutability class? However, due to your argument, at least if I understand it correctly, this should work.

There is a technical reason with incompatible slots in the Cython classes. However, this was not the original intent of this class. The reason is Sage vectors are Cython classes, not Python classes, which only have single inheritance. So they cannot have mixin classes. All of this is done to optimize them because linear algebra needs to be fast.

[mjungmath]

Changed dependencies from #30181, #30266, #30274 to #30181, #30266, #30274, #30280

[mjungmath]

Changed dependencies from #30181, #30266, #30274, #30280 to #30181, #30266, #30274, #30280, #30284

[mkoeppe]

comment:36

Sage development has entered the release candidate phase for 9.3. Setting a new milestone for this ticket based on a cursory review of ticket status, priority, and last modification date.

[mjungmath]

Changed dependencies from #30181, #30266, #30274, #30280, #30284, #30288, #30310, #30311 to #30181, #30266, #30274, #30280, #30284, #30288, #30310, #30311, #31706

[slel]

comment:41

By positive review, do you mean all goals of this meta-ticket are complete?

We might want to wait for #31706 to be merged before closing here.

Once that is done, maybe set the milestone here to sage-duplicate/invalid/wontfix?

[mkoeppe]

comment:42

Setting a new milestone for this ticket based on a cursory review.