Opened 15 months ago

Closed 7 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:

Status badges

Description (last modified by gh-mjungmath)

Immutability of chart functions, see #30261.

Change History (27)

comment:1 Changed 15 months ago by gh-mjungmath

  • Branch set to u/gh-mjungmath/immutability_of_chart_functions

comment:2 Changed 15 months ago by gh-mjungmath

  • Authors set to Michael Jung
  • 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

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 15 months ago by gh-mjungmath

  • Dependencies set to #30181

comment:4 Changed 15 months ago by egourgoulhon

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: Changed 15 months ago by 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):

comment:6 Changed 15 months ago by gh-mjungmath

ModuleElementWithMutability is first introduced in #30181.

comment:7 Changed 15 months ago by tscrim

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

comment:8 Changed 15 months ago by gh-mjungmath

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

comment:9 Changed 15 months ago by tscrim

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 15 months ago by egourgoulhon

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 14 months ago by mkoeppe

  • Milestone changed from sage-9.2 to sage-9.3

comment:12 Changed 10 months ago by gh-mjungmath

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 10 months ago by gh-mjungmath (previous) (diff)

comment:13 Changed 10 months ago by gh-mjungmath

Last edited 10 months ago by gh-mjungmath (previous) (diff)

comment:14 Changed 10 months ago by gh-mjungmath

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 9 months ago by git

  • Commit changed from 76229bd5c1111d77dc7c98da97e96c0cc653fcda to 09086cf653e0ce2d26757733189ff70561f783fd

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

09086cfTrac #30310: inherit from Mutability

comment:16 Changed 9 months ago by gh-mjungmath

  • Dependencies changed from #30181 to #31181, #31182

comment:17 Changed 9 months ago by git

  • Commit changed from 09086cf653e0ce2d26757733189ff70561f783fd to 6c76b66df5a673560dd700f8a942af07da6e6d19

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

6c76b66Trac #30310: AssertionError -> ValueError

comment:18 Changed 9 months ago by git

  • Commit changed from 6c76b66df5a673560dd700f8a942af07da6e6d19 to 848e96bf4fed67afe74d434cb4d7f946d2cc595c

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 9 months ago by gh-mjungmath

Green patchbot.

comment:20 Changed 9 months ago by tscrim

I think you are better off explicitly calling

ModuleElementWithMutability.__init__(self, parent)

comment:21 Changed 9 months ago by gh-mjungmath

Yes thanks, this should be better.

comment:22 Changed 9 months ago by gh-mjungmath

Pushed.

comment:23 Changed 9 months ago by git

  • Commit changed from 848e96bf4fed67afe74d434cb4d7f946d2cc595c to e855ebc9c793e8ca9e66b3cb667758fc1b3bcaa3

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

e855ebcTrac #30310: init ModuleElementWithMutability directly

comment:24 Changed 9 months ago by tscrim

  • Reviewers set to Travis Scrimshaw

Green bot => positive review.

comment:25 Changed 9 months ago by tscrim

  • Status changed from needs_review to positive_review

comment:26 Changed 9 months ago by gh-mjungmath

Thank you.

comment:27 Changed 7 months ago by vbraun

  • Branch changed from u/gh-mjungmath/immutability_of_chart_functions to e855ebc9c793e8ca9e66b3cb667758fc1b3bcaa3
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.