Opened 2 years ago
Closed 2 years 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, 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 2 years ago by
- Description modified (diff)
comment:2 Changed 2 years ago by
- Branch set to u/mkoeppe/immutable_elements_of_freemoduletensor
comment:3 Changed 2 years ago by
- Commit set to 9df3d22ca84da6b604226d5b88d2a581b0cc53ca
comment:4 Changed 2 years 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 2 years ago by
- Status changed from new to needs_review
comment:6 Changed 2 years ago by
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: ↓ 9 Changed 2 years 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 2 years 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 2 years 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 2 years 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 2 years 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 2 years ago by
Thanks!
comment:13 Changed 2 years 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 follow-up: ↓ 23 Changed 2 years 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 follow-up: ↓ 16 Changed 2 years ago by
@gh-mjungmath Did you see comment:8?
comment:16 in reply to: ↑ 15 ; follow-up: ↓ 17 Changed 2 years ago by
comment:17 in reply to: ↑ 16 ; follow-ups: ↓ 18 ↓ 22 Changed 2 years ago by
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: ↓ 19 Changed 2 years ago by
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.
comment:19 in reply to: ↑ 18 ; follow-up: ↓ 20 Changed 2 years ago by
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.
comment:20 in reply to: ↑ 19 ; follow-up: ↓ 21 Changed 2 years 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 2 years ago by
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: ↓ 24 Changed 2 years ago by
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 2 years ago by
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: ↓ 25 Changed 2 years ago by
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 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 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
andCommutativeAlgebraElement
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: ↓ 26 Changed 2 years ago by
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
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 ; follow-up: ↓ 27 Changed 2 years ago by
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
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.
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 2 years ago by
Replying to gh-mjungmath:
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 2 years 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 2 years 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