Opened 5 months ago
Last modified 3 months ago
#31901 new enhancement
Chart: Implement pickling via __getstate__/__setstate__
Reported by:  mkoeppe  Owned by:  

Priority:  major  Milestone:  sagewishlist 
Component:  manifolds  Keywords:  
Cc:  egourgoulhon, tscrim, ghmjungmath  Merged in:  
Authors:  Reviewers:  
Report Upstream:  N/A  Work issues:  redo on top of #31894 
Branch:  u/mkoeppe/chart__no_longer_use_uniquerepresentation__implement___getstate_____setstate__ (Commits, GitHub, GitLab)  Commit:  30a5d0ba21f277044eebe7dfe6e3b463bcd0c5c5 
Dependencies:  #31894  Stopgaps: 
Description (last modified by )
In this ticket, we implement the pickling protocol using __getstate__
and __setstate__
Change History (43)
comment:1 Changed 4 months ago by
 Description modified (diff)
 Summary changed from Chart: UniqueRepresentation fixes to Chart: No longer use UniqueRepresentation; implement __getstate__/__setstate__
comment:2 in reply to: ↑ description ; followup: ↓ 4 Changed 4 months ago by
comment:3 Changed 4 months ago by
Some thought: if UniqueRepresentation
is abandoned for charts, it may be desirable to maintain WithEqualityById
(one of the two mother classes of UniqueRepresentation
) to have a fast equality operator. Indeed charts are heavily used as dictionary keys: for coordinates of manifold points, for coordinate expressions of scalar fields (the latter being used, among other things, to store the components of tensor fields), for coordinate expressions of continuous maps between manifolds (the keys being pairs of charts in that case), etc. Certainly WithEqualityById
is the fastest equality operator and thus provides a fast access to the above dictionaries.
comment:4 in reply to: ↑ 2 Changed 4 months ago by
Replying to egourgoulhon:
Replying to mkoeppe:
sage: D = Manifold(2, 'D') sage: X.<x,y> = D.chart(restrictions=x^2 + y^2 < 1)would generate
NameError: name 'y' is not defined
. Of course, one could replacex^2 + y^2 < 1
by the string"x^2 + y^2 < 1"
, but this is not very userfriendly. Do you already have some solution in mind?
Ah! That's of course a problem, I didn't realize this
comment:5 Changed 4 months ago by
 Description modified (diff)
comment:6 followup: ↓ 14 Changed 4 months ago by
Maybe a chart should become immutable "upon first use"?
comment:7 Changed 4 months ago by
 Branch set to u/mkoeppe/chart__no_longer_use_uniquerepresentation__implement___getstate_____setstate__
comment:8 Changed 4 months ago by
 Commit set to 5215e93549135b378736ad9da655af2577bf06bf
 Description modified (diff)
New commits:
5215e93  Chart: WIP WithEqualityById instead of UniqueRepresentation; disambiguate _restrictions > _coordinate_restrictions

comment:9 Changed 4 months ago by
 Commit changed from 5215e93549135b378736ad9da655af2577bf06bf to 869599d9b2177ae7376cf8480ff0aafb01239563
Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
facad97  ContinuousMap.preimage: Handle identity_map specially

c2ecf3e  ScalarField.preimage: Handle the case of the zero scalar field

3b1c428  ManifoldSubsetPullback: Make codomain_subset required 2nd init arg; fix pycodestyle/pyflakes warnings

d321b93  ContinuousMap: Return domain if the map's codomain is contained in the given subset

07aba9e  ManifoldSubsetPullback.is_closed: Preimage of finite sets is closed

653c651  src/sage/manifolds/subsets/pullback.py: Fix docstring markup

0f2e018  Merge #31688

e3e38ca  Chart.__init__: Move code that attaches self to the domain to TopologicalManifold.chart

3581c75  Chart.function_ring: Cache via @cached_method, not UniqueRepresentation

869599d  Chart: WIP equality/immutability/pickling

comment:10 Changed 4 months ago by
 Dependencies set to #31688
comment:11 Changed 4 months ago by
 Commit changed from 869599d9b2177ae7376cf8480ff0aafb01239563 to d071d56c3c25abb47f04ad9731dfd08c188c22c7
Branch pushed to git repo; I updated commit sha1. New commits:
d071d56  TopologicalSubmanifold: Adjust to renamed chart attribute _coord_restrictions

comment:12 Changed 4 months ago by
The last obstacle is this code which tries to hash a chart when it is still mutable.
# The null and one functions of the coordinates: # Expression in self of the zero and one scalar fields of open sets # containing the domain of self: for dom in self.open_supersets(): dom._zero_scalar_field._express[chart] = chart.function_ring().zero() dom._one_scalar_field._express[chart] = chart.function_ring().one()
comment:13 Changed 4 months ago by
 Commit changed from d071d56c3c25abb47f04ad9731dfd08c188c22c7 to 7b822b4f5efbb138bddfd779051fe0c29b73d7c7
Branch pushed to git repo; I updated commit sha1. New commits:
7b822b4  Fixup

comment:14 in reply to: ↑ 6 Changed 4 months ago by
Replying to mkoeppe:
Maybe a chart should become immutable "upon first use"?
This is the approach that I have been trying out in this branch. But it does not work in examples like this one:
sage: M = Manifold(2, 'M', structure='topological') sage: X.<x,y> = M.chart() sage: U = M.open_subset('U', coord_def={X: [x>1, y<pi]})
because X
in coord_def
is unhashable, leading to an error.
So perhaps an API change is needed, after all...
comment:15 followups: ↓ 17 ↓ 18 Changed 4 months ago by
In the current branch, Chart
inherits from WithEqualityById
but redefines __eq__
(in a rather complicated and possibly CPU time consuming way). Doesn't this defeat the purpose? In particular in view of comment:3.
In the code of __hash__
, isn't
if not self.is_immutable(): raise TypeError('Charts are unhashable until set_immutable() has been called')
an unnecessary limitation?
More generally, why do you want to make Chart
mutable? It is logically immutable: a chart is entirely defined by its domain and its coordinates (i.e. a tuple of symbolic variables). Coordinate restrictions, which can be added only after __init__
via add_restrictions
in the current implementation, do not (mathematically) modify the object: there cannot be two charts with the same domain and the same coordinates, but with different coordinate restrictions.
comment:16 followup: ↓ 19 Changed 4 months ago by
A possible option to get rid of the 2stages construction of a chart (i.e. to get rid of the requirement of invoking add_restriction
after __init__
) could be to add the keyword argument restrictions
to Chart.__init__
, which can be either
 a string (case with no predefined coordinate variables, i.e. chart created from scratch)
 a nested list/tuple of symbolic expressions (case of a chart created from another one, as in
Chart.restrict
)
After all, when creating a chart from scratch, we are already passing a string if we want to specify the coordinate ranges and LaTeX symbols; so maybe we could admit having a second string... For instance, we could have something like
sage: H = Manifold(2, 'H') # halfdisk sage: X.<x,y> = H.chart(r"x y:(0,+oo)", restrictions="x^2 + y^2 < 1")
or equivalently
sage: X.<x,y> = H.chart(restrictions="[x^2 + y^2 < 1, y>0]")
Then we could get rid of the method add_restriction
and consider that the chart is a fully immutable object (of course, technically speaking, the sheafy attributes can be muted).
comment:17 in reply to: ↑ 15 Changed 4 months ago by
Replying to egourgoulhon:
In the current branch,
Chart
inherits fromWithEqualityById
but redefines__eq__
(in a rather complicated and possibly CPU time consuming way). Doesn't this defeat the purpose?
Yes, this part is not finished. I started with WithEqualityById
as per your suggestion, but I wanted to implement actual pickling (not just passing the picklingrelated testsuite) as a step to actual pickling of manifolds. Then the identity and uniquerepresentation tricks are not sufficient any more, and one needs to implement full comparison.
One can still have a fast path for equality using id, and fast lookup in sets etc. via a fast hash.
comment:18 in reply to: ↑ 15 Changed 4 months ago by
Replying to egourgoulhon:
More generally, why do you want to make
Chart
mutable?
I don't really, it was just an attempt to keep the current API (initialization, followed by add_restrictions
) but implementing pickling.
comment:19 in reply to: ↑ 16 ; followup: ↓ 21 Changed 4 months ago by
When discussing a possible redesign of chart initialization, I think a relevant issue is the current use of the symbolic assumptions facility for the coordinates.
Are two charts with a different tuple of variables allowed to share some variables? If yes, does the shared variable have to have the same global assumptions and periodicity?
Also I note that creating a RealChart
puts global assumptions on the variables of the chart. So creating a subchart will affect simplification of expressions that relate to computation on the original chart. This should probably be revised.
comment:20 Changed 4 months ago by
 Commit changed from 7b822b4f5efbb138bddfd779051fe0c29b73d7c7 to 96e1fbf0e91282303a7996a3e33788a8e9dd5daa
Branch pushed to git repo; I updated commit sha1. New commits:
4b930ff  Merge tag '9.4.beta4' into t/31688/pullbacks_of_manifold_subsets_under_continuous_maps

96e1fbf  Merge branch 't/31688/pullbacks_of_manifold_subsets_under_continuous_maps' into t/31901/chart__no_longer_use_uniquerepresentation__implement___getstate_____setstate__

comment:21 in reply to: ↑ 19 ; followups: ↓ 22 ↓ 23 ↓ 25 ↓ 28 ↓ 36 ↓ 38 Changed 4 months ago by
Replying to mkoeppe:
When discussing a possible redesign of chart initialization, I think a relevant issue is the current use of the symbolic assumptions facility for the coordinates.
Are two charts with a different tuple of variables allowed to share some variables?
Yes, definitely. This is a desirable feature.
If yes, does the shared variable have to have the same global assumptions and periodicity?
No (see below).
Also I note that creating a
RealChart
puts global assumptions on the variables of the chart. So creating a subchart will affect simplification of expressions that relate to computation on the original chart. This should probably be revised.
Yes, absolutely. This is something I have in mind for a long time but did not find the time to work on it. The assumptions should not be global but chartwise. The way Sage's symbolic calculus is implemented, I guess this means that they should be switched on (via assume
) before each calculus at the chart function level and switched off once the calculus is done (via forget
).
A connected question is how to pass these chartbased assumptions to SymPy, in case the latter has been chosen as the default symbolic backend on the manifold (via M.set_calculus_method('sympy')
) or on a given chart X
(via X.calculus_method().set('sympy')
). For the time being, calculus with SymPy is possible but the assumptions are not taken into account on the SymPy side...
comment:22 in reply to: ↑ 21 ; followup: ↓ 26 Changed 4 months ago by
Replying to egourgoulhon:
Replying to mkoeppe:
When discussing a possible redesign of chart initialization, I think a relevant issue is the current use of the symbolic assumptions facility for the coordinates.
Are two charts with a different tuple of variables allowed to share some variables?
Yes, definitely. This is a desirable feature.
If yes, does the shared variable have to have the same global assumptions and periodicity?
No (see below).
OK. Then I think there is little point in trying to provide better syntax for creating the variables than asking the user to do var('x, y, z')
before creating a chart with coordinate restrictions.
Then the restrictions can just be passed as symbolic inequalities to the constructor. (I don't think going the string route would be an improvement.)
We could deprecate add_restrictions
then.
comment:23 in reply to: ↑ 21 ; followup: ↓ 27 Changed 4 months ago by
Replying to egourgoulhon:
Replying to mkoeppe:
Also I note that creating a
RealChart
puts global assumptions on the variables of the chart. [...]Yes, absolutely. This is something I have in mind for a long time but did not find the time to work on it. The assumptions should not be global but chartwise. The way Sage's symbolic calculus is implemented, I guess this means that they should be switched on (via
assume
) before each calculus at the chart function level and switched off once the calculus is done (viaforget
).
There is already a context manager assuming
for that. We could create it at initialization and invoke it using with
whenever computations are done with this chart.
The tricky bit is when two charts are involved with overlapping variables. Then some renaming may have to be done first. A possible route is through #32089.
comment:24 Changed 4 months ago by
I have opened #32102 (Chart
: Add init argument coord_restrictions
, deprecate method add_restrictions
)
comment:25 in reply to: ↑ 21 Changed 4 months ago by
Replying to egourgoulhon:
A connected question is how to pass these chartbased assumptions to SymPy, in case the latter has been chosen as the default symbolic backend on the manifold (via
M.set_calculus_method('sympy')
) or on a given chartX
(viaX.calculus_method().set('sympy')
). For the time being, calculus with SymPy is possible but the assumptions are not taken into account on the SymPy side...
I suppose we can go through a new method assuming
of CalculusMethod
that dispatches in the same way as the simplify
method does.
comment:26 in reply to: ↑ 22 ; followup: ↓ 29 Changed 4 months ago by
Replying to mkoeppe:
OK. Then I think there is little point in trying to provide better syntax for creating the variables than asking the user to do
var('x, y, z')
before creating a chart with coordinate restrictions.
The issue here is that on real manifolds (by far the most used ones), the symbolic variables are created with the extra parameter domain='real'
(cf. RealChart._init_coordinates
), so the user must actually do var('x y z', domain='real')
, which is not very intuitive. For some reason, it is not equivalent to assume(x, 'real')
, which is performed in RealChart._init_coordinates
as well. Both proved to be necessary for some simplifications.
Then the restrictions can just be passed as symbolic inequalities to the constructor. (I don't think going the string route would be an improvement.)
I don't like strings either, but I would prefer this to a 2stage declaration involving var
. Anyway, let us continue the discussion on #32102.
We could deprecate
add_restrictions
then.
Yes.
comment:27 in reply to: ↑ 23 ; followup: ↓ 37 Changed 4 months ago by
Replying to mkoeppe:
Replying to egourgoulhon:
Replying to mkoeppe:
Also I note that creating a
RealChart
puts global assumptions on the variables of the chart. [...]Yes, absolutely. This is something I have in mind for a long time but did not find the time to work on it. The assumptions should not be global but chartwise. The way Sage's symbolic calculus is implemented, I guess this means that they should be switched on (via
assume
) before each calculus at the chart function level and switched off once the calculus is done (viaforget
).There is already a context manager
assuming
for that. We could create it at initialization and invoke it usingwith
whenever computations are done with this chart.The tricky bit is when two charts are involved with overlapping variables. Then some renaming may have to be done first.
I don't see why: two computations on two distinct charts can set different assumptions on the same variable. There is no ambiguity as long as assumptions are tied to charts.
comment:28 in reply to: ↑ 21 ; followup: ↓ 31 Changed 4 months ago by
Replying to egourgoulhon:
Replying to mkoeppe:
When discussing a possible redesign of chart initialization, I think a relevant issue is the current use of the symbolic assumptions facility for the coordinates.
Are two charts with a different tuple of variables allowed to share some variables?
Yes, definitely. This is a desirable feature.
A followup question on this:
sage: M = Manifold(2, 'M', structure='topological') ....: U = M.open_subset('U') ....: V = M.open_subset('V') sage: XU = U.chart(names=("x", "y")) sage: XV = V.chart(names=("x", "y")) sage: M.atlas() [Chart (U, (x, y)), Chart (V, (x, y))] sage: M.top_charts() [Chart (U, (x, y))]
Clearly something went wrong here.
Should there have been a declaration of something on M
regarding the intention to declare charts with the coordinate pair (x,y)
on subsets?
comment:29 in reply to: ↑ 26 Changed 4 months ago by
Replying to egourgoulhon:
Replying to mkoeppe: I think there is little point in trying to provide better syntax for creating the variables than asking the user to do
var('x, y, z')
before creating a chart with coordinate restrictions.The issue here is that on real manifolds (by far the most used ones), the symbolic variables are created with the extra parameter
domain='real'
(cf.RealChart._init_coordinates
), so the user must actually dovar('x y z', domain='real')
, which is not very intuitive.
How about something like M.var('x y z')
then?
comment:30 Changed 4 months ago by
 Dependencies changed from #31688 to #31688, #32102
comment:31 in reply to: ↑ 28 Changed 4 months ago by
Replying to mkoeppe:
A followup question on this:
sage: M = Manifold(2, 'M', structure='topological') ....: U = M.open_subset('U') ....: V = M.open_subset('V') sage: XU = U.chart(names=("x", "y")) sage: XV = V.chart(names=("x", "y")) sage: M.atlas() [Chart (U, (x, y)), Chart (V, (x, y))] sage: M.top_charts() [Chart (U, (x, y))]Clearly something went wrong here.
Good catch!
Should there have been a declaration of something on
M
regarding the intention to declare charts with the coordinate pair(x,y)
on subsets?
No, the above is actually a bug in Chart.__init__
. I've opened #32112 for it and submitted a fix there.
comment:32 Changed 4 months ago by
 Commit changed from 96e1fbf0e91282303a7996a3e33788a8e9dd5daa to d2e3ff79b13cec3ff8a20218c4bb4a2bdbc47fc4
comment:33 Changed 4 months ago by
 Dependencies changed from #31688, #32102 to #31688, #32112, #32102
comment:34 Changed 4 months ago by
 Commit changed from d2e3ff79b13cec3ff8a20218c4bb4a2bdbc47fc4 to a328e9199ae63de7976d2ccf4951a2172083ffbf
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
a328e91  Merge #32112

comment:35 Changed 4 months ago by
 Work issues set to redo without mutability on top of #32102
comment:36 in reply to: ↑ 21 Changed 4 months ago by
Replying to egourgoulhon:
creating a
RealChart
puts global assumptions on the variables of the chart. So creating a subchart will affect simplification of expressions that relate to computation on the original chart.The assumptions should not be global but chartwise.
I've opened #32120 (Chartwise assumptions) for this.
comment:37 in reply to: ↑ 27 ; followup: ↓ 40 Changed 4 months ago by
Replying to egourgoulhon:
Replying to mkoeppe:
The tricky bit is when two charts are involved with overlapping variables. Then some renaming may have to be done first.
I don't see why: two computations on two distinct charts can set different assumptions on the same variable. There is no ambiguity as long as assumptions are tied to charts.
I was thinking of computations with coordinate changes here, which involve two charts simultaneously.
comment:38 in reply to: ↑ 21 Changed 4 months ago by
Replying to egourgoulhon:
A connected question is how to pass these chartbased assumptions to SymPy, in case the latter has been chosen as the default symbolic backend on the manifold (via
M.set_calculus_method('sympy')
) or on a given chartX
(viaX.calculus_method().set('sympy')
). For the time being, calculus with SymPy is possible but the assumptions are not taken into account on the SymPy side...
We already have a metaticket for this, #31958 (Use the SymPy assumptions facility). I was surprised that SymPy's assumptions are not really well connected to SymPy?'s sets (for which I have been building some glue code in #31926).
comment:39 Changed 4 months ago by
 Milestone changed from sage9.4 to sagewishlist
Given that #32102 is making mutability unnecessary, it seems we can keep UniqueRepresentation
for charts for now (until we see another reason to drop it  such as unbounded memory) and pickle through that.
I'm setting the ticket to "sagewishlist" for this reason.
comment:40 in reply to: ↑ 37 Changed 4 months ago by
Replying to mkoeppe:
Replying to egourgoulhon:
Replying to mkoeppe:
The tricky bit is when two charts are involved with overlapping variables. Then some renaming may have to be done first.
I don't see why: two computations on two distinct charts can set different assumptions on the same variable. There is no ambiguity as long as assumptions are tied to charts.
I was thinking of computations with coordinate changes here, which involve two charts simultaneously.
There should not by any issue with coordinate changes: a coordinate change X1 > X2
involves only a set of chart functions based on X1
(stored as a MultiCoordFunction
in the attribute CoordChange._transf
). So only assumptions relative to X1
will be invoked in calculus involving the coordinate change X1 > X2
(such as the Jacobian matrix). An exception is the
computation of the inverse in CoordChange.inverse
. There only assumptions on X2
should matter. However, if X1
and X2
share the same variables, the inverse is trivial. To summarize, I don't think there is a case where both sets of assumptions are required simultaneously in coordinate changes.
comment:41 Changed 4 months ago by
Thanks for the explanation, that's good news.
comment:42 Changed 3 months ago by
 Commit changed from a328e9199ae63de7976d2ccf4951a2172083ffbf to 30a5d0ba21f277044eebe7dfe6e3b463bcd0c5c5
Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:
bf62543  Merge branch 't/32089/conditionset__conditionset_callable_symbolic_expression' into t/32009/eliminate_direct_use_of_the_chart__domain_attribute

2f47485  Merge #32009

21297f3  Merge #32009

deace83  Chart: Replace _init_coordinates by _parse_coordinates

4db4995  Chart: Fix up __classcall__ and _parse_coordinates by avoiding unhashable things

fc59c9d  Chart.__classcall__: Add doctest

1742fdc  Merge #32009

9ac1834  src/sage/manifolds/chart.py: Add raw string marker

d7f9d17  Merge #32116

30a5d0b  Chart: WIP getstate/setstate, no UniqueRepresentation

comment:43 Changed 3 months ago by
 Dependencies changed from #31688, #32112, #32102 to #31894
 Description modified (diff)
 Summary changed from Chart: No longer use UniqueRepresentation; implement __getstate__/__setstate__ to Chart: Implement pickling via __getstate__/__setstate__
 Work issues changed from redo without mutability on top of #32102 to redo on top of #31894
Replying to mkoeppe:
This would be nice! In the early stages of the manifold project, I did not manage to do it simply, i.e. without changing Sage's preparser, since
would generate
NameError: name 'y' is not defined
. Of course, one could replacex^2 + y^2 < 1
by the string"x^2 + y^2 < 1"
, but this is not very userfriendly. Do you already have some solution in mind?