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: sage-9.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:

Status badges

Description (last modified by Michael Jung)

Immutability of chart functions, see #30261.

Change History (27)

comment:1 Changed 2 years ago by Michael Jung

Branch: u/gh-mjungmath/immutability_of_chart_functions

comment:2 Changed 2 years ago by Michael Jung

Authors: Michael Jung
Cc: Eric Gourgoulhon Travis Scrimshaw Matthias Köppe added
Commit: 76229bd5c1111d77dc7c98da97e96c0cc653fcda
Component: PLEASE CHANGEmanifolds
Description: modified (diff)
Keywords: immutability added
Status: newneeds_review
Type: PLEASE CHANGEenhancement

New commits:

286d3c6FreeModuleTensor, Vector: Subclass a new class ModuleElementWithMutability
9df3d22sage.tensor.modules: Make all zero() and one() elements immutable
4373ea2FreeModuleTensor: Replace special casing of the immutable zero by is_immutable
a2ee8beModuleElementWithMutability.is_immutable, is_mutable: Change def to cpdef
8600c2dMerge branch 't/30181/immutable_elements_of_freemoduletensor' into t/30310/immutability_of_chart_functions
76229bdTrac #30310: immutability of chart functions

comment:3 Changed 2 years ago by Michael Jung

Dependencies: #30181

comment:4 Changed 2 years ago by Eric Gourgoulhon

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 Changed 2 years ago by Michael Jung

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 Michael Jung

ModuleElementWithMutability is first introduced in #30181.

comment:7 Changed 2 years ago by Travis Scrimshaw

I feel like the MultiCoordFunction should inherit from Mutability rather than copy the same code.

comment:8 Changed 2 years ago by Michael Jung

It'd be the same issue like we had for affine connections (#30280): __reduce__ (#30281).

comment:9 Changed 2 years ago by Travis Scrimshaw

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 2 years ago by Eric Gourgoulhon

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 2 years ago by Matthias Köppe

Milestone: sage-9.2sage-9.3

comment:12 Changed 2 years ago by Michael Jung

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?

Last edited 2 years ago by Michael Jung (previous) (diff)

comment:13 Changed 2 years ago by Michael Jung

Last edited 2 years ago by Michael Jung (previous) (diff)

comment:14 Changed 2 years ago by Michael Jung

I opened a ticket for the missing return: #31181.

Furthermore, I've opened a ticket for the still remaining pickling test error: #31182.

comment:15 Changed 2 years ago by git

Commit: 76229bd5c1111d77dc7c98da97e96c0cc653fcda09086cf653e0ce2d26757733189ff70561f783fd

Branch pushed to git repo; I updated commit sha1. New commits:

09086cfTrac #30310: inherit from Mutability

comment:16 Changed 2 years ago by Michael Jung

Dependencies: #30181#31181, #31182

comment:17 Changed 23 months ago by git

Commit: 09086cf653e0ce2d26757733189ff70561f783fd6c76b66df5a673560dd700f8a942af07da6e6d19

Branch pushed to git repo; I updated commit sha1. New commits:

6c76b66Trac #30310: AssertionError -> ValueError

comment:18 Changed 23 months ago by git

Commit: 6c76b66df5a673560dd700f8a942af07da6e6d19848e96bf4fed67afe74d434cb4d7f946d2cc595c

Branch pushed to git repo; I updated commit sha1. New commits:

7247620Merge branch 'develop' into t/30310/immutability_of_chart_functions
5ecbf3aTrac #31181: return added
8b20fc4Merge branch 't/31181/mutability_class_does_not_return_is_immutable' into t/30310/immutability_of_chart_functions
4c33935Trac #31196: minor code improvements, py3 compatibility, documentation improved
e5228d3Trac #31196: cpdef require methods + example added
d957f73Trac #31196: unnecessary line in docstring removed
d6d6ba4Trac #31182: __getstate__ and __setstate__
6cbd1fdTrac #31182: doctests added for __setstate__ and __getstate__
848e96bMerge branch 't/31182/mutability_class_and_pickling' into t/30310/immutability_of_chart_functions

comment:19 Changed 23 months ago by Michael Jung

Green patchbot.

comment:20 Changed 23 months ago by Travis Scrimshaw

I think you are better off explicitly calling

ModuleElementWithMutability.__init__(self, parent)

comment:21 Changed 23 months ago by Michael Jung

Yes thanks, this should be better.

comment:22 Changed 23 months ago by Michael Jung

Pushed.

comment:23 Changed 23 months ago by git

Commit: 848e96bf4fed67afe74d434cb4d7f946d2cc595ce855ebc9c793e8ca9e66b3cb667758fc1b3bcaa3

Branch pushed to git repo; I updated commit sha1. New commits:

e855ebcTrac #30310: init ModuleElementWithMutability directly

comment:24 Changed 23 months ago by Travis Scrimshaw

Reviewers: Travis Scrimshaw

Green bot => positive review.

comment:25 Changed 23 months ago by Travis Scrimshaw

Status: needs_reviewpositive_review

comment:26 Changed 23 months ago by Michael Jung

Thank you.

comment:27 Changed 21 months ago by Volker Braun

Branch: u/gh-mjungmath/immutability_of_chart_functionse855ebc9c793e8ca9e66b3cb667758fc1b3bcaa3
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.