Opened 2 years ago
Closed 21 months ago
#30310 closed enhancement (fixed)
Immutability of chart functions
Reported by:  Michael Jung  Owned by:  

Priority:  major  Milestone:  sage9.3 
Component:  manifolds  Keywords:  immutability 
Cc:  Eric Gourgoulhon, Travis Scrimshaw, Matthias Köppe  Merged in:  
Authors:  Michael Jung  Reviewers:  Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  e855ebc (Commits, GitHub, GitLab)  Commit:  e855ebc9c793e8ca9e66b3cb667758fc1b3bcaa3 
Dependencies:  #31181, #31182  Stopgaps: 
Description (last modified by )
Immutability of chart functions, see #30261.
Change History (27)
comment:1 Changed 2 years ago by
Branch:  → u/ghmjungmath/immutability_of_chart_functions 

comment:2 Changed 2 years ago by
Authors:  → Michael Jung 

Cc:  Eric Gourgoulhon Travis Scrimshaw Matthias Köppe added 
Commit:  → 76229bd5c1111d77dc7c98da97e96c0cc653fcda 
Component:  PLEASE CHANGE → manifolds 
Description:  modified (diff) 
Keywords:  immutability added 
Status:  new → needs_review 
Type:  PLEASE CHANGE → enhancement 
comment:3 Changed 2 years ago by
Dependencies:  → #30181 

comment:4 Changed 2 years ago by
Why is #30181 a dependency of this ticket? A priori chart functions are totally independent from tensor fields. They even exist on pure topological manifolds.
comment:5 followup: 10 Changed 2 years ago by
It is because of
from sage.structure.element import AlgebraElement +from sage.structure.element import AlgebraElement, ModuleElementWithMutability ... class ChartFunction(AlgebraElement): +class ChartFunction(AlgebraElement, ModuleElementWithMutability):
comment:7 Changed 2 years ago by
I feel like the MultiCoordFunction
should inherit from Mutability
rather than copy the same code.
comment:8 Changed 2 years ago by
comment:9 Changed 2 years ago by
I know, but as a stopgap, you could instead implement a __reduce__
. I don't really like this duplication. Hopefully this week I can get around to properly doing #30281...
comment:10 Changed 2 years ago by
Replying to ghmjungmath:
It is because of
from sage.structure.element import AlgebraElement +from sage.structure.element import AlgebraElement, ModuleElementWithMutability ... class ChartFunction(AlgebraElement): +class ChartFunction(AlgebraElement, ModuleElementWithMutability):
Ah yes, thanks.
comment:11 Changed 2 years ago by
Milestone:  sage9.2 → sage9.3 

comment:12 Changed 2 years ago by
I tried to remove the redundant code and inherit from Mutability
after #30281. For some reason, I still get a pickling error during doctest.
Furthermore, the code seems broken: is_immutable
does not return anything:
def is_immutable(self): """ ... """ self._is_immutable
When did that happen?
comment:14 Changed 2 years ago by
comment:15 Changed 2 years ago by
Commit:  76229bd5c1111d77dc7c98da97e96c0cc653fcda → 09086cf653e0ce2d26757733189ff70561f783fd 

Branch pushed to git repo; I updated commit sha1. New commits:
09086cf  Trac #30310: inherit from Mutability

comment:16 Changed 2 years ago by
Dependencies:  #30181 → #31181, #31182 

comment:17 Changed 23 months ago by
Commit:  09086cf653e0ce2d26757733189ff70561f783fd → 6c76b66df5a673560dd700f8a942af07da6e6d19 

Branch pushed to git repo; I updated commit sha1. New commits:
6c76b66  Trac #30310: AssertionError > ValueError

comment:18 Changed 23 months ago by
Commit:  6c76b66df5a673560dd700f8a942af07da6e6d19 → 848e96bf4fed67afe74d434cb4d7f946d2cc595c 

Branch pushed to git repo; I updated commit sha1. New commits:
7247620  Merge branch 'develop' into t/30310/immutability_of_chart_functions

5ecbf3a  Trac #31181: return added

8b20fc4  Merge branch 't/31181/mutability_class_does_not_return_is_immutable' into t/30310/immutability_of_chart_functions

4c33935  Trac #31196: minor code improvements, py3 compatibility, documentation improved

e5228d3  Trac #31196: cpdef require methods + example added

d957f73  Trac #31196: unnecessary line in docstring removed

d6d6ba4  Trac #31182: __getstate__ and __setstate__

6cbd1fd  Trac #31182: doctests added for __setstate__ and __getstate__

848e96b  Merge branch 't/31182/mutability_class_and_pickling' into t/30310/immutability_of_chart_functions

comment:20 Changed 23 months ago by
I think you are better off explicitly calling
ModuleElementWithMutability.__init__(self, parent)
comment:23 Changed 23 months ago by
Commit:  848e96bf4fed67afe74d434cb4d7f946d2cc595c → e855ebc9c793e8ca9e66b3cb667758fc1b3bcaa3 

Branch pushed to git repo; I updated commit sha1. New commits:
e855ebc  Trac #30310: init ModuleElementWithMutability directly

comment:25 Changed 23 months ago by
Status:  needs_review → positive_review 

comment:27 Changed 21 months ago by
Branch:  u/ghmjungmath/immutability_of_chart_functions → e855ebc9c793e8ca9e66b3cb667758fc1b3bcaa3 

Resolution:  → fixed 
Status:  positive_review → closed 
New commits:
FreeModuleTensor, Vector: Subclass a new class ModuleElementWithMutability
sage.tensor.modules: Make all zero() and one() elements immutable
FreeModuleTensor: Replace special casing of the immutable zero by is_immutable
ModuleElementWithMutability.is_immutable, is_mutable: Change def to cpdef
Merge branch 't/30181/immutable_elements_of_freemoduletensor' into t/30310/immutability_of_chart_functions
Trac #30310: immutability of chart functions