Opened 4 months ago

Last modified 5 weeks ago

#30261 new task

Meta-ticket: Immutability for manifold objects

Reported by: gh-mjungmath Owned by:
Priority: major Milestone: sage-9.3
Component: manifolds Keywords: immutability
Cc: egourgoulhon, mkoeppe, tscrim Merged in:
Authors: Reviewers:
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #30181, #30266, #30274, #30280, #30284, #30288, #30310, #30311 Stopgaps:

Description (last modified by gh-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.

Change History (30)

comment:1 Changed 4 months ago by gh-mjungmath

  • Description modified (diff)

comment:2 Changed 4 months ago by gh-mjungmath

  • Description modified (diff)

comment:3 Changed 4 months ago by gh-mjungmath

  • Description modified (diff)

comment:4 Changed 4 months ago by gh-mjungmath

  • Description modified (diff)

comment:5 Changed 4 months ago by gh-mjungmath

  • Dependencies set to #30181

comment:6 follow-up: Changed 4 months ago by gh-mjungmath

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'

comment:7 Changed 4 months ago by mkoeppe

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

comment:8 in reply to: ↑ 6 ; follow-up: Changed 4 months ago by tscrim

Replying to gh-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.

comment:9 in reply to: ↑ 8 ; follow-up: Changed 4 months ago by gh-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.

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

comment:10 in reply to: ↑ 9 ; follow-up: Changed 4 months ago by tscrim

Replying to gh-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.

comment:11 in reply to: ↑ 10 ; follow-up: Changed 4 months ago by gh-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?

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.

Last edited 4 months ago by gh-mjungmath (previous) (diff)

comment:12 Changed 4 months ago by gh-mjungmath

  • Dependencies changed from #30181 to #30181, #30266

comment:13 Changed 4 months ago by gh-mjungmath

  • Description modified (diff)

comment:14 Changed 4 months ago by gh-mjungmath

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

comment:15 Changed 4 months ago by gh-mjungmath

  • Type changed from defect to task

comment:16 Changed 4 months ago by gh-mjungmath

  • Dependencies changed from #30181, #30266 to #30181, #30266, #30274
  • Description modified (diff)

comment:17 Changed 4 months ago by gh-mjungmath

  • Description modified (diff)

comment:18 in reply to: ↑ 11 ; follow-up: Changed 4 months ago by tscrim

Replying to gh-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).

comment:19 in reply to: ↑ 18 ; follow-up: Changed 4 months ago by gh-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?

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.

comment:20 in reply to: ↑ 19 Changed 4 months ago by tscrim

Replying to gh-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.

comment:21 Changed 4 months ago by gh-mjungmath

  • Dependencies changed from #30181, #30266, #30274 to #30181, #30266, #30274, #30280
  • Description modified (diff)

comment:22 Changed 4 months ago by gh-mjungmath

  • Dependencies changed from #30181, #30266, #30274, #30280 to #30181, #30266, #30274, #30280, #30284
  • Description modified (diff)

comment:23 Changed 4 months ago by gh-mjungmath

  • Description modified (diff)

comment:24 Changed 4 months ago by gh-mjungmath

  • Dependencies changed from #30181, #30266, #30274, #30280, #30284 to #30181, #30266, #30274, #30280, #30284, #30288
  • Description modified (diff)

comment:25 Changed 4 months ago by gh-mjungmath

  • Dependencies changed from #30181, #30266, #30274, #30280, #30284, #30288 to #30181, #30266, #30274, #30280, #30284, #30288, #30310
  • Description modified (diff)
  • Keywords immutability added; immutable removed

comment:26 Changed 4 months ago by gh-mjungmath

  • Dependencies changed from #30181, #30266, #30274, #30280, #30284, #30288, #30310 to #30181, #30266, #30274, #30280, #30284, #30288, #30310, 30311
  • Description modified (diff)

comment:27 Changed 4 months ago by gh-mjungmath

  • Dependencies changed from #30181, #30266, #30274, #30280, #30284, #30288, #30310, 30311 to #30181, #30266, #30274, #30280, #30284, #30288, #30310, #30311

comment:28 Changed 4 months ago by gh-mjungmath

That should be all. In case I missed something, please let me know.

comment:29 Changed 4 months ago by mkoeppe

  • Summary changed from Immutability for manifold objects to Meta-ticket: Immutability for manifold objects

comment:30 Changed 5 weeks ago by mkoeppe

  • Milestone changed from sage-9.2 to sage-9.3
Note: See TracTickets for help on using tickets.