Immutable elements of FreeModuleTensor

[mkoeppe]

Currently, elements of a FiniteRankFreeModule and its tensor modules are mutable and therefore cannot be hashed.

In analogy to sage.modules.FreeModuleElement and sage.matrix.Matrix, a method set_immutable() should be added.

In this ticket, we do this by creating a subclass ModuleElementWithMutability of ModuleElement. The existing classes Vector (and thus FreeModuleElement from sage.modules) and FreeModuleTensor (from sage.tensor) are changed to subclass it.

The methods from FreeModuleElement implementing mutability are moved to ModuleElementWithMutability and become available for FreeModuleTensor in this way. (The class Vector also gets them for free.)

[git]

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

​9df3d22

sage.tensor.modules: Make all zero() and one() elements immutable

[git]

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

​4373ea2

FreeModuleTensor: Replace special casing of the immutable zero by is_immutable

[mkoeppe]

Although hashing is one of the motivations for this ticket, it will not be implemented on this ticket. A follow-up ticket will take care of it.

[tscrim]

I would also make those methods

cpdef bool is_[im]mutable(self):

In some ways, I think this would be better as a mixin class. Alas, Cython does not have multiple inheritance...

[mkoeppe]

There is already a mixin class written in Cython, sage.structure.mutability, but it cannot be used for exactly this reason.

[mkoeppe]

Replying to tscrim:

I would also make those methods

cpdef bool is_[im]mutable(self):

Yes, I'll make this change

[git]

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

​a2ee8be

ModuleElementWithMutability.is_immutable, is_mutable: Change def to cpdef

[tscrim]

Thank you. LGTM.

(I slightly wonder if a user who really cares about speed should instead be just breaking the encapsulation here...)

[mkoeppe]

Thanks!

[gh-mjungmath]

Thank you for this very nice improvement!

What about the name of an immutable element? Should that be immutable, too? This is not covered in this modification, but maybe it should?

As you suggested a systematic solution to #30239, it would be good to add this improvement to all modules in the manifolds module and to the scalar field algebra as well. The latter probably needs a new implementation, e.g. CommutativeAlgebraElementWithMutability.

[gh-mjungmath]

What about a slighly more general approach like having a class ElementWithMutability and then simply

class FreeModuleTensor(ModuleElement, ElementWithMutability) 

This approach can also be applied to other elements. The hashablility can then be implemented via ElementWithMutability.

This has also the great advantage that this can be used for affine connections and bundle connections, too.

[tscrim]

@gh-mjungmath Did you see comment:8?

[gh-mjungmath]

Replying to tscrim:

@gh-mjungmath Did you see comment:8?

Oh, I see. But is implementing the same code for each kind of element really necessary?

[tscrim]

Replying to gh-mjungmath:

Replying to tscrim:

@gh-mjungmath Did you see comment:8?

Oh, I see. But is implementing the same code for each kind of element really necessary?

Cython does not have multiple inheritance and you want the Sage vectors, which are Cython classes, to have this behavior. Since that structure is already there, the FreeModuleTensor should use it.

[gh-mjungmath]

Replying to tscrim:

Replying to gh-mjungmath:

Replying to tscrim:

@gh-mjungmath Did you see comment:8?

Oh, I see. But is implementing the same code for each kind of element really necessary?

Cython does not have multiple inheritance and you want the Sage vectors, which are Cython classes, to have this behavior. Since that structure is already there, the FreeModuleTensor should use it.

Reasonable point. Mh, that would mean, we need a seperate solution for scalar fields and connections, right?

Do you have an opinion regarding the behavior of the tensor's names? I think that the names should be fixed as soon as the element is immutable.

[tscrim]

Replying to gh-mjungmath:

Replying to tscrim:

Replying to gh-mjungmath:

Replying to tscrim:

@gh-mjungmath Did you see comment:8?

Oh, I see. But is implementing the same code for each kind of element really necessary?

Cython does not have multiple inheritance and you want the Sage vectors, which are Cython classes, to have this behavior. Since that structure is already there, the FreeModuleTensor should use it.

Reasonable point. Mh, that would mean, we need a seperate solution for scalar fields and connections, right?

Possibly, but it might suggest that you just need a better mixin class that is perhaps (a subclass of) sage.structure.mutability. I am not sure as you know the code better than I do. You can try experimenting and seeing what works best.

Do you have an opinion regarding the behavior of the tensor's names? I think that the names should be fixed as soon as the element is immutable.

Part of me would want it to be mutable because I want to rename things, but the other part says that 0 should not be renamed. I would say because of that, (latex) names should be immutable.

[gh-mjungmath]

Replying to tscrim:

Part of me would want it to be mutable because I want to rename things, but the other part says that 0 should not be renamed. I would say because of that, (latex) names should be immutable.

This is exactly the reason why I struggle, too.

However, I think, the names should be set fixed when the element is immutable. At least, immutability means exactly this.

Matthias, what do you say? Would you mind to add this condition here?

Anyway, I would use your code as a template to apply the mutability changes to the manifolds module accordingly; also to fix #30239 "more systematically", as you said.

[mkoeppe]

Replying to gh-mjungmath:

However, I think, the names should be set fixed when the element is immutable. At least, immutability means exactly this.

Matthias, what do you say?

I feel I don't really have a qualified opinion on this because I have not studied how you use these symbolic names in sage-manifolds.

The vectors from sage.modules do not support renaming, so there's no concern regarding compatibility here.

sage: v = vector(QQ, [2, 3, 4])
sage: v.rename('mangrove')
NotImplementedError: object does not support renaming: (2, 3, 4)

Neither do elements of CombinatorialFreeModule.

Would you mind to add this condition here?

I would prefer it if this could be done on follow-up tickets to keep this technical ticket small.

There are some more changes that you could consider: vector supports an optional argument immutable; and hashing of immutable tensors should be implemented.

Anyway, I would use your code as a template to apply the mutability changes to the manifolds module accordingly; also to fix #30239 "more systematically", as you said.

Great!

[mkoeppe]

Replying to tscrim:

Replying to gh-mjungmath:

Replying to tscrim:

@gh-mjungmath Did you see comment:8?

Oh, I see. But is implementing the same code for each kind of element really necessary?

Cython does not have multiple inheritance and you want the Sage vectors, which are Cython classes, to have this behavior. Since that structure is already there, the FreeModuleTensor should use it.

Actually this is not quite true. One cannot do general mix-in classes with Cython. But the following works:

diff --git a/src/sage/manifolds/scalarfield.py b/src/sage/manifolds/scalarfield.py
index edb52ecd95..83883394f2 100644
--- a/src/sage/manifolds/scalarfield.py
+++ b/src/sage/manifolds/scalarfield.py
@@ -40,12 +40,12 @@ REFERENCES:
 #                  https://www.gnu.org/licenses/
 # *****************************************************************************
 
-from sage.structure.element import CommutativeAlgebraElement
+from sage.structure.element import CommutativeAlgebraElement, ModuleElementWithMutability
 from sage.symbolic.expression import Expression
 from sage.manifolds.chart_func import ChartFunction
 
 
-class ScalarField(CommutativeAlgebraElement):
+class ScalarField(CommutativeAlgebraElement, ModuleElementWithMutability):
     r"""
     Scalar field on a topological manifold.
 

This works because between none of the intermediate classes between ModuleElement and CommutativeAlgebraElement add slots.

sage: M = Manifold(2, 'M', structure='topological') # the 2-dimensional sphere S^2
sage: U = M.open_subset('U') # complement of the North pole
sage: c_xy.<x,y> = U.chart() # stereographic coordinates from the North pole
sage: g = U.scalar_field(x*y, chart=c_xy, name='g') ; g
Scalar field g on the Open subset U of the 2-dimensional topological manifold M
sage: g.set_immutable()
sage: g.is_immutable()
True

[mkoeppe]

Replying to gh-mjungmath:

What about a slighly more general approach like having a class ElementWithMutability

I think I wouldn't like to push the mutability down to the level of Element. Mutating components is something that we traditionally do with "vectors". Other types of elements are traditionally immutable, and a functional style (creating new objects instead of mutating objects) is preferred.

[gh-mjungmath]

Replying to mkoeppe:

Would you mind to add this condition here?

I would prefer it if this could be done on follow-up tickets to keep this technical ticket small.

This makes sense.

There are some more changes that you could consider: vector supports an optional argument immutable; and hashing of immutable tensors should be implemented.

That's right. More importantly, connections already support hashing even though they cannot be considered immutable. Strictly speaking, this is a defect.

Replying to mkoeppe:

Actually this is not quite true. One cannot do general mix-in classes with Cython. But the following works:

diff --git a/src/sage/manifolds/scalarfield.py b/src/sage/manifolds/scalarfield.py
index edb52ecd95..83883394f2 100644
--- a/src/sage/manifolds/scalarfield.py
+++ b/src/sage/manifolds/scalarfield.py
@@ -40,12 +40,12 @@ REFERENCES:
 #                  https://www.gnu.org/licenses/
 # *****************************************************************************
 
-from sage.structure.element import CommutativeAlgebraElement
+from sage.structure.element import CommutativeAlgebraElement, ModuleElementWithMutability
 from sage.symbolic.expression import Expression
 from sage.manifolds.chart_func import ChartFunction
 
 
-class ScalarField(CommutativeAlgebraElement):
+class ScalarField(CommutativeAlgebraElement, ModuleElementWithMutability):
     r"""
     Scalar field on a topological manifold.
 

This works because between none of the intermediate classes between ModuleElement and CommutativeAlgebraElement add slots.

sage: M = Manifold(2, 'M', structure='topological') # the 2-dimensional sphere S^2
sage: U = M.open_subset('U') # complement of the North pole
sage: c_xy.<x,y> = U.chart() # stereographic coordinates from the North pole
sage: g = U.scalar_field(x*y, chart=c_xy, name='g') ; g
Scalar field g on the Open subset U of the 2-dimensional topological manifold M
sage: g.set_immutable()
sage: g.is_immutable()
True

Do you think this is a reasonable way to go? I am afraid that potential changes in the intermediated classes might break this whole construct down...

[mkoeppe]

Replying to gh-mjungmath:

connections already support hashing even though they cannot be considered immutable. Strictly speaking, this is a defect.

Yes, I think this is a defect.

Replying to mkoeppe:

diff --git a/src/sage/manifolds/scalarfield.py b/src/sage/manifolds/scalarfield.py
-class ScalarField(CommutativeAlgebraElement):
+class ScalarField(CommutativeAlgebraElement, ModuleElementWithMutability):

This works because between none of the intermediate classes between ModuleElement and CommutativeAlgebraElement add slots.

Do you think this is a reasonable way to go?

Yes. But actually - should an AffineConnection not be an element of some module too?

I am afraid that potential changes in the intermediated classes might break this whole construct down...

Don't worry about this. It would be highly controversial to add additional slots to these classes.

[gh-mjungmath]

Replying to mkoeppe:

Replying to gh-mjungmath:

connections already support hashing even though they cannot be considered immutable. Strictly speaking, this is a defect.

Yes, I think this is a defect.

Replying to mkoeppe:

diff --git a/src/sage/manifolds/scalarfield.py b/src/sage/manifolds/scalarfield.py
-class ScalarField(CommutativeAlgebraElement):
+class ScalarField(CommutativeAlgebraElement, ModuleElementWithMutability):

This works because between none of the intermediate classes between ModuleElement and CommutativeAlgebraElement add slots.

Do you think this is a reasonable way to go?

Yes. But actually - should an AffineConnection not be an element of some module too?

Actually, on the second thought, I don't think that connections constitute a vector space. There is no zero element since this would contradict the Leibniz rule.

I wonder why Mutability does not inherit from SageObject. If it would, one could let connections directly inherit from Mutability. Is there a particular reason?

[mkoeppe]

Replying to gh-mjungmath:

I wonder why Mutability does not inherit from SageObject. If it would, one could let connections directly inherit from Mutability. Is there a particular reason?

Hard to know. It was added in #14524. I don't know if it was ever used

[gh-mjungmath]

The invokation of __init__ via super() here seems to cause some troubles on the level of manifolds. Anyway, I think the discussion can be closed and should be shifted to #30261. I will add the dependence.