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

Priority:  major  Milestone:  sage9.3 
Component:  manifolds  Keywords:  immutability 
Cc:  egourgoulhon, tscrim, mkoeppe  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 set to u/ghmjungmath/immutability_of_chart_functions
comment:2 Changed 2 years ago by
 Cc egourgoulhon tscrim mkoeppe added
 Commit set to 76229bd5c1111d77dc7c98da97e96c0cc653fcda
 Component changed from PLEASE CHANGE to manifolds
 Description modified (diff)
 Keywords immutability added
 Status changed from new to needs_review
 Type changed from PLEASE CHANGE to enhancement
comment:3 Changed 2 years ago by
 Dependencies set to #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:6 Changed 2 years ago by
ModuleElementWithMutability
is first introduced in #30181.
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 in reply to: ↑ 5 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 changed from sage9.2 to sage9.3
comment:12 Changed 20 months 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 20 months ago by
comment:15 Changed 19 months ago by
 Commit changed from 76229bd5c1111d77dc7c98da97e96c0cc653fcda to 09086cf653e0ce2d26757733189ff70561f783fd
Branch pushed to git repo; I updated commit sha1. New commits:
09086cf  Trac #30310: inherit from Mutability

comment:16 Changed 19 months ago by
 Dependencies changed from #30181 to #31181, #31182
comment:17 Changed 19 months ago by
 Commit changed from 09086cf653e0ce2d26757733189ff70561f783fd to 6c76b66df5a673560dd700f8a942af07da6e6d19
Branch pushed to git repo; I updated commit sha1. New commits:
6c76b66  Trac #30310: AssertionError > ValueError

comment:18 Changed 19 months ago by
 Commit changed from 6c76b66df5a673560dd700f8a942af07da6e6d19 to 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:19 Changed 19 months ago by
Green patchbot.
comment:20 Changed 19 months ago by
I think you are better off explicitly calling
ModuleElementWithMutability.__init__(self, parent)
comment:21 Changed 19 months ago by
Yes thanks, this should be better.
comment:22 Changed 19 months ago by
Pushed.
comment:23 Changed 19 months ago by
 Commit changed from 848e96bf4fed67afe74d434cb4d7f946d2cc595c to e855ebc9c793e8ca9e66b3cb667758fc1b3bcaa3
Branch pushed to git repo; I updated commit sha1. New commits:
e855ebc  Trac #30310: init ModuleElementWithMutability directly

comment:25 Changed 19 months ago by
 Status changed from needs_review to positive_review
comment:26 Changed 19 months ago by
Thank you.
comment:27 Changed 17 months ago by
 Branch changed from u/ghmjungmath/immutability_of_chart_functions to e855ebc9c793e8ca9e66b3cb667758fc1b3bcaa3
 Resolution set to fixed
 Status changed from positive_review to 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