Opened 12 months ago
Closed 12 months ago
#30181 closed enhancement (fixed)
Immutable elements of FreeModuleTensor
Reported by:  mkoeppe  Owned by:  

Priority:  major  Milestone:  sage9.2 
Component:  linear algebra  Keywords:  
Cc:  egourgoulhon, tscrim, ghmjungmath  Merged in:  
Authors:  Matthias Koeppe  Reviewers:  Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  a2ee8be (Commits, GitHub, GitLab)  Commit:  a2ee8beb504477a3215e002391e44a51b2957ca2 
Dependencies:  Stopgaps: 
Description (last modified by )
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 12 months ago by
 Description modified (diff)
comment:2 Changed 12 months ago by
 Branch set to u/mkoeppe/immutable_elements_of_freemoduletensor
comment:3 Changed 12 months ago by
 Commit set to 9df3d22ca84da6b604226d5b88d2a581b0cc53ca
comment:4 Changed 12 months ago by
 Commit changed from 9df3d22ca84da6b604226d5b88d2a581b0cc53ca to 4373ea269d9d426163fa3d1e72bdf6e20c412f87
Branch pushed to git repo; I updated commit sha1. New commits:
4373ea2  FreeModuleTensor: Replace special casing of the immutable zero by is_immutable

comment:5 Changed 12 months ago by
 Status changed from new to needs_review
comment:6 Changed 12 months ago by
Although hashing is one of the motivations for this ticket, it will not be implemented on this ticket. A followup ticket will take care of it.
comment:7 followup: ↓ 9 Changed 12 months ago by
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 12 months ago by
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 12 months ago by
Replying to tscrim:
I would also make those methods
cpdef bool is_[im]mutable(self):
Yes, I'll make this change
comment:10 Changed 12 months ago by
 Commit changed from 4373ea269d9d426163fa3d1e72bdf6e20c412f87 to a2ee8beb504477a3215e002391e44a51b2957ca2
Branch pushed to git repo; I updated commit sha1. New commits:
a2ee8be  ModuleElementWithMutability.is_immutable, is_mutable: Change def to cpdef

comment:11 Changed 12 months ago by
 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 12 months ago by
Thanks!
comment:13 Changed 12 months ago by
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 followup: ↓ 23 Changed 12 months ago by
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 followup: ↓ 16 Changed 12 months ago by
@ghmjungmath Did you see comment:8?
comment:16 in reply to: ↑ 15 ; followup: ↓ 17 Changed 12 months ago by
comment:17 in reply to: ↑ 16 ; followups: ↓ 18 ↓ 22 Changed 12 months ago by
Replying to ghmjungmath:
Replying to tscrim:
@ghmjungmath 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 ; followup: ↓ 19 Changed 12 months ago by
Replying to tscrim:
Replying to ghmjungmath:
Replying to tscrim:
@ghmjungmath 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.
comment:19 in reply to: ↑ 18 ; followup: ↓ 20 Changed 12 months ago by
Replying to ghmjungmath:
Replying to tscrim:
Replying to ghmjungmath:
Replying to tscrim:
@ghmjungmath 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.
comment:20 in reply to: ↑ 19 ; followup: ↓ 21 Changed 12 months ago by
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.
comment:21 in reply to: ↑ 20 Changed 12 months ago by
Replying to ghmjungmath:
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 sagemanifolds.
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 followup 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 ; followup: ↓ 24 Changed 12 months ago by
Replying to tscrim:
Replying to ghmjungmath:
Replying to tscrim:
@ghmjungmath 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 mixin 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 2dimensional 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 2dimensional topological manifold M sage: g.set_immutable() sage: g.is_immutable() True
comment:23 in reply to: ↑ 14 Changed 12 months ago by
Replying to ghmjungmath:
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 ; followup: ↓ 25 Changed 12 months ago by
Replying to mkoeppe:
Would you mind to add this condition here?
I would prefer it if this could be done on followup tickets to keep this technical ticket small.
This makes sense.
There are some more changes that you could consider:
vector
supports an optional argumentimmutable
; 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 mixin 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
andCommutativeAlgebraElement
add slots.sage: M = Manifold(2, 'M', structure='topological') # the 2dimensional 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 2dimensional 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 ; followup: ↓ 26 Changed 12 months ago by
Replying to ghmjungmath:
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
andCommutativeAlgebraElement
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 ; followup: ↓ 27 Changed 12 months ago by
Replying to mkoeppe:
Replying to ghmjungmath:
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
andCommutativeAlgebraElement
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?
comment:27 in reply to: ↑ 26 Changed 12 months ago by
Replying to ghmjungmath:
I wonder why
Mutability
does not inherit fromSageObject
. If it would, one could let connections directly inherit fromMutability
. 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 12 months ago by
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 12 months ago by
 Branch changed from u/mkoeppe/immutable_elements_of_freemoduletensor to a2ee8beb504477a3215e002391e44a51b2957ca2
 Resolution set to fixed
 Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. New commits:
sage.tensor.modules: Make all zero() and one() elements immutable