Opened 23 months ago
Closed 16 months ago
#30310 closed enhancement (fixed)
Immutability of chart functions
Reported by: | gh-mjungmath | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-9.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 23 months ago by
- Branch set to u/gh-mjungmath/immutability_of_chart_functions
comment:2 Changed 23 months 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 23 months ago by
- Dependencies set to #30181
comment:4 Changed 23 months 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 follow-up: ↓ 10 Changed 23 months 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 23 months ago by
ModuleElementWithMutability
is first introduced in #30181.
comment:7 Changed 23 months ago by
I feel like the MultiCoordFunction
should inherit from Mutability
rather than copy the same code.
comment:8 Changed 23 months ago by
comment:9 Changed 23 months ago by
I know, but as a stop-gap, 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 23 months ago by
Replying to gh-mjungmath:
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 22 months ago by
- Milestone changed from sage-9.2 to sage-9.3
comment:12 Changed 18 months ago by
I tried to remove the redundant code and inherit from Mutability
. 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:13 Changed 18 months ago by
comment:14 Changed 18 months ago by
comment:15 Changed 18 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 18 months ago by
- Dependencies changed from #30181 to #31181, #31182
comment:17 Changed 17 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 17 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 17 months ago by
Green patchbot.
comment:20 Changed 17 months ago by
I think you are better off explicitly calling
ModuleElementWithMutability.__init__(self, parent)
comment:21 Changed 17 months ago by
Yes thanks, this should be better.
comment:22 Changed 17 months ago by
Pushed.
comment:23 Changed 17 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 17 months ago by
- Status changed from needs_review to positive_review
comment:26 Changed 17 months ago by
Thank you.
comment:27 Changed 16 months ago by
- Branch changed from u/gh-mjungmath/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