Opened 3 months ago

Closed 3 months ago

#30181 closed enhancement (fixed)

Immutable elements of FreeModuleTensor

Reported by: mkoeppe Owned by:
Priority: major Milestone: sage-9.2
Component: linear algebra Keywords:
Cc: egourgoulhon, tscrim, gh-mjungmath Merged in:
Authors: Matthias Koeppe Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: a2ee8be (Commits) Commit: a2ee8beb504477a3215e002391e44a51b2957ca2
Dependencies: Stopgaps:

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

Change History (29)

comment:1 Changed 3 months ago by mkoeppe

  • Authors set to Matthias Koeppe
  • Description modified (diff)

comment:2 Changed 3 months ago by mkoeppe

  • Branch set to u/mkoeppe/immutable_elements_of_freemoduletensor

comment:3 Changed 3 months ago by git

  • Commit set to 9df3d22ca84da6b604226d5b88d2a581b0cc53ca

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

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

comment:4 Changed 3 months ago by git

  • Commit changed from 9df3d22ca84da6b604226d5b88d2a581b0cc53ca to 4373ea269d9d426163fa3d1e72bdf6e20c412f87

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

4373ea2FreeModuleTensor: Replace special casing of the immutable zero by is_immutable

comment:5 Changed 3 months ago by mkoeppe

  • Status changed from new to needs_review

comment:6 Changed 3 months ago by 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.

comment:7 follow-up: Changed 3 months ago by 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...

comment:8 Changed 3 months ago by mkoeppe

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

comment:9 in reply to: ↑ 7 Changed 3 months ago by mkoeppe

Replying to tscrim:

I would also make those methods

cpdef bool is_[im]mutable(self):

Yes, I'll make this change

comment:10 Changed 3 months ago by git

  • Commit changed from 4373ea269d9d426163fa3d1e72bdf6e20c412f87 to a2ee8beb504477a3215e002391e44a51b2957ca2

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

a2ee8beModuleElementWithMutability.is_immutable, is_mutable: Change def to cpdef

comment:11 Changed 3 months ago by tscrim

  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

Thank you. LGTM.

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

comment:12 Changed 3 months ago by mkoeppe

Thanks!

comment:13 Changed 3 months ago by 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.

comment:14 follow-up: Changed 3 months ago by 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.

comment:15 follow-up: Changed 3 months ago by tscrim

@gh-mjungmath Did you see comment:8?

comment:16 in reply to: ↑ 15 ; follow-up: Changed 3 months ago by 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?

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

comment:17 in reply to: ↑ 16 ; follow-ups: Changed 3 months ago by 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.

comment:18 in reply to: ↑ 17 ; follow-up: Changed 3 months ago by 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.

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

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

Last edited 3 months ago by tscrim (previous) (diff)

comment:20 in reply to: ↑ 19 ; follow-up: Changed 3 months ago by 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.

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

comment:21 in reply to: ↑ 20 Changed 3 months ago by 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!

comment:22 in reply to: ↑ 17 ; follow-up: Changed 3 months ago by 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

comment:23 in reply to: ↑ 14 Changed 3 months ago by 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.

comment:24 in reply to: ↑ 22 ; follow-up: Changed 3 months ago by 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...

comment:25 in reply to: ↑ 24 ; follow-up: Changed 3 months ago by 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.

comment:26 in reply to: ↑ 25 ; follow-up: Changed 3 months ago by 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. Furthermore, in fact, only convex linear combinations are connections again.

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?

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

comment:27 in reply to: ↑ 26 Changed 3 months ago by 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

comment:28 Changed 3 months ago by 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.

comment:29 Changed 3 months ago by vbraun

  • Branch changed from u/mkoeppe/immutable_elements_of_freemoduletensor to a2ee8beb504477a3215e002391e44a51b2957ca2
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.