Opened 4 months ago
Closed 3 months ago
#32102 closed enhancement (fixed)
Chart: Add init argument coord_restrictions, deprecate method add_restrictions
Reported by:  mkoeppe  Owned by:  

Priority:  major  Milestone:  sage9.4 
Component:  manifolds  Keywords:  
Cc:  egourgoulhon, ghmjungmath, tscrim, vbraun  Merged in:  
Authors:  Matthias Koeppe  Reviewers:  Eric Gourgoulhon 
Report Upstream:  N/A  Work issues:  
Branch:  u/mkoeppe/chart__add_init_argument_coord_restrictions__deprecate_method_add_restrictions (Commits, GitHub, GitLab)  Commit:  c218828e5bfea9d5cb75d29cc4e864961397989c 
Dependencies:  #32116, #32009  Stopgaps: 
Description (last modified by )
(from https://trac.sagemath.org/ticket/31901#comment:22)
This will remove the appearance of mutability of Chart
s.
Deprecated chart declaration:
sage: M = Manifold(2, 'M', structure='topological') sage: X.<x,y> = M.chart() sage: X.add_restrictions(x^2+y^2<1) sage: X.add_restrictions(x>0)
Replacement 1 (implemented):
sage: var("x y") sage: X.<x,y> = M.chart(coord_restrictions=[x^2+y^2<1, x>0])
Replacement 2:
sage: x, y = M.var() sage: X.<x,y> = M.chart(coord_restrictions=[x^2+y^2<1, x>0])
(x
, y
defined in the first line would be some temporary placeholder variables.)
Replacement 3 (implemented):
sage: in_disk(x,y) = x^2+y^2<1 sage: on_right(x,y) = x>0 sage: X.<x,y> = M.chart(coord_restrictions=[in_disk, on_right])
Replacement 4:
sage: X.<x,y> = M.chart(coord_restrictions=[lambda x,y: x^2+y^2<1, lambda x,y: x>0])
(chart initialization calls the lambdas on the symbolic variables x, y to get the symbolic relations)
Replacement 5 (implemented):
sage: X.<x,y> = M.chart(coord_restrictions=lambda x,y: [x^2+y^2<1, x>0])
(chart initialization calls the lambda on the symbolic variables x, y to get the list of symbolic relations)
Change History (41)
comment:1 Changed 4 months ago by
 Dependencies set to #32089
comment:2 Changed 4 months ago by
 Description modified (diff)
comment:3 Changed 4 months ago by
 Dependencies changed from #32089 to #32089, #32103
comment:4 Changed 4 months ago by
comment:5 followup: ↓ 13 Changed 4 months ago by
OK, converting a lambda to a callable symbolic expression is in #32103, but I guess for this ticket one could just call the lambda on the chart variables, so it's not really needed.
I was expecting trouble with UniqueRepresentation
because the mixture of lists and tuples in your format of coordinate restrictions is not hashable. My suggestion was to convert this to a ConditionSet
(#32089); but I suppose simpler solutions are possible too
comment:6 Changed 4 months ago by
The lambda
in the init args will unfortunately also create trouble with the pickling tests  lambdas cannot be pickled. So it will have to be transformed to the list/tuple format already in the __classcall__
method  triggering the trouble with UniqueRepresentation
...
So we can either go through #32089, or get rid of UniqueRepresentation
for Chart
s.
comment:7 Changed 4 months ago by
 Branch set to u/mkoeppe/chart__add_init_argument_coord_restrictions__deprecate_method_add_restrictions
comment:8 followup: ↓ 9 Changed 4 months ago by
 Commit set to d9520ae9989dfdccefdf3f20f45389cb4f632dc5
comment:9 in reply to: ↑ 8 Changed 4 months ago by
 Dependencies changed from #32089, #32103 to #32116, #32089, #32103
comment:10 Changed 4 months ago by
 Commit changed from d9520ae9989dfdccefdf3f20f45389cb4f632dc5 to ce765863d3c5d81a6460e5e2f7fd305d690337fb
Branch pushed to git repo; I updated commit sha1. New commits:
8ba174c  Eliminate direct use of Chart._domain

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

ce76586  Merge #32116

comment:11 Changed 4 months ago by
 Commit changed from ce765863d3c5d81a6460e5e2f7fd305d690337fb to c31fb4baf8cefdff7a9864e774941caae697e603
Branch pushed to git repo; I updated commit sha1. New commits:
c31fb4b  Chart: Make coord restrictions hashable by using frozenset in place of list

comment:12 Changed 4 months ago by
 Dependencies changed from #32116, #32089, #32103 to #32116, #32103
comment:13 in reply to: ↑ 5 Changed 4 months ago by
Replying to mkoeppe:
I was expecting trouble with
UniqueRepresentation
because the mixture of lists and tuples in your format of coordinate restrictions is not hashable. My suggestion was to convert this to aConditionSet
(#32089); but I suppose simpler solutions are possible too
I have implemented a simpler solution
comment:14 Changed 4 months ago by
 Commit changed from c31fb4baf8cefdff7a9864e774941caae697e603 to 30bc60dffdadae7db53364edd7f650d1a3c24d49
comment:15 Changed 4 months ago by
Replacements 1, 2, 3, and 5 should work now. For simplicity, let's skip 4.
comment:16 Changed 4 months ago by
 Dependencies changed from #32116, #32103 to #32116
comment:17 Changed 4 months ago by
 Commit changed from 30bc60dffdadae7db53364edd7f650d1a3c24d49 to b5820a9aabf558768caccfcbbfaec0f679cb5d26
Branch pushed to git repo; I updated commit sha1. New commits:
b5820a9  Remove most uses of Chart.add_restrictions in doctests

comment:18 Changed 4 months ago by
 Commit changed from b5820a9aabf558768caccfcbbfaec0f679cb5d26 to 1e482307cd15ce4f40cfdfd1d477db3693fd5ab0
Branch pushed to git repo; I updated commit sha1. New commits:
1e48230  RealChart._tighten_bounds: Factor out from RealChart.add_restrictions, call it from __init__ too

comment:19 Changed 4 months ago by
 Commit changed from 1e482307cd15ce4f40cfdfd1d477db3693fd5ab0 to 939dcd5c3884adad6c757b0ce6cb0840f26b73d6
Branch pushed to git repo; I updated commit sha1. New commits:
3a5217c  Chart._normalize_coord_restrictions: Move resolution of lambdas here

4c8e6d3  OpenInterval: Remove use of add_restrictions

f8c537d  IntegratedCurve: Remove use of add_restrictions in doctests

939dcd5  Chart.add_restrictions: Deprecate

comment:20 Changed 4 months ago by
 Cc ghmjungmath tscrim added
 Status changed from new to needs_review
There is one remaining use of add_restrictions
in TopologicalSubmanifold.adapted_chart
, but it is not triggered by any doctest.
Because this code also uses assume
, it is probably best to defer work on it to a followup ticket.
comment:21 Changed 4 months ago by
 Dependencies changed from #32116 to #32116, #32009
comment:22 Changed 4 months ago by
 Commit changed from 939dcd5c3884adad6c757b0ce6cb0840f26b73d6 to 0dfc22e151a7fbcf26d5bb06ce6bc36f5dee366a
Branch pushed to git repo; I updated commit sha1. New commits:
f43b358  Chart._init_coordinates: Fix up use of domain

4e316e9  Fix bug in Chart.__init__ regarding the determination of top charts (Trac #32112)

907c9bc  Merge #32112

0dfc22e  Merge branch 't/32009/eliminate_direct_use_of_the_chart__domain_attribute' into t/32102/chart__add_init_argument_coord_restrictions__deprecate_method_add_restrictions

comment:23 Changed 4 months ago by
 Commit changed from 0dfc22e151a7fbcf26d5bb06ce6bc36f5dee366a to 0e63ff14b415d468ee43578c512fa48f7813f983
comment:24 Changed 4 months ago by
 Commit changed from 0e63ff14b415d468ee43578c512fa48f7813f983 to f66e4bc00b87b87196743273ec8108c3b86fada8
comment:25 Changed 4 months ago by
 Description modified (diff)
comment:26 followup: ↓ 28 Changed 3 months ago by
Looks good. Here are some remarks:
 The argument
calc_method
ofChart.__init__
has no longer a default value. Is there any reason for this? This is not consistent withRealChart.__init__
, which hascalc_method=None
.
 In the INPUT section of the docstring of class
Chart
:
 the argument
coordinates
is described as having a default value, which is no longer true  in the description of the argument
coord_restrictions
, all instances ofrestrictions
should be replaced bycoord_restrictions
 the argument
names
should removed
Moreover, the arguments should be ordered in the same way as they appear in Chart.__init__
.
 In the INPUT section of the docstring of class
RealChart
:
 the argument
coordinates
is described as having a default value, which is no longer true  the argument
coord_restrictions
is missing  the argument
names
should be removed
 In the INPUT section of the docstring of
TopologicalManifold.chart
, the argumentcoord_restrictions
is missing; also it could be nice to illustrate its use with a lambda function in the corresponding EXAMPLE section. Actually, this is the first piece of documentation about charts that the user encounters when typingM.chart?
.
comment:27 Changed 3 months ago by
 Commit changed from f66e4bc00b87b87196743273ec8108c3b86fada8 to 7c003dba37f63144d36ec37109deb5b9dadeb9eb
Branch pushed to git repo; I updated commit sha1. New commits:
7c003db  Chart.__init__: Restore default of calc_method argument for consistency

comment:28 in reply to: ↑ 26 ; followup: ↓ 30 Changed 3 months ago by
Replying to egourgoulhon:
 In the INPUT section of the docstring of class
Chart
:
 the argument
coordinates
is described as having a default value, which is no longer true[...]
 the argument
names
should removed
Actually, coordinates
and names
still have the same behavior as before  it is implemented by __classcall__
comment:29 Changed 3 months ago by
 Commit changed from 7c003dba37f63144d36ec37109deb5b9dadeb9eb to a39e6fcd968af4a05c777357cdd470e61c244114
Branch pushed to git repo; I updated commit sha1. New commits:
f85e710  Chart: in the description of the argument coord_restrictions, replace all instances of 'restrictions' by 'coord_restrictions'

80f6195  Chart, RealChart: In class docstring, order arguments as they appear in __classcall__/__init__

a39e6fc  DiffChart, RealDiffChart: In class docstring, order arguments as they appear in __classcall__/__init__; add description of argument coord_restrictions

comment:30 in reply to: ↑ 28 ; followup: ↓ 33 Changed 3 months ago by
Replying to mkoeppe:
Replying to egourgoulhon:
 In the INPUT section of the docstring of class
Chart
:
 the argument
coordinates
is described as having a default value, which is no longer true[...]
 the argument
names
should removedActually,
coordinates
andnames
still have the same behavior as before  it is implemented by__classcall__
Yes I understand this; the question is rather about the consistency of the INPUT field with the class declaration as it appears in the reference manual:
class sage.manifolds.chart.Chart(domain, coordinates, calc_method, periods=None, coord_restrictions=None)
This is because Sphinx constructs the above from the __init__
method, not taking into account __classcall__
. If one looks at the html output of the reference manual, the INPUT field arrives just after the class declaration. But I agree with you that this does not reflect the way the class should be invoked. I don't know what is the policy here. Possibly, this issue appears in other parts of Sage as well.
New commits:
f85e710  Chart: in the description of the argument coord_restrictions, replace all instances of 'restrictions' by 'coord_restrictions'

80f6195  Chart, RealChart: In class docstring, order arguments as they appear in __classcall__/__init__

a39e6fc  DiffChart, RealDiffChart: In class docstring, order arguments as they appear in __classcall__/__init__; add description of argument coord_restrictions

comment:31 Changed 3 months ago by
 Commit changed from a39e6fcd968af4a05c777357cdd470e61c244114 to cdf20b04b24497e66279b5b745f4ef1ca9e7b555
Branch pushed to git repo; I updated commit sha1. New commits:
cdf20b0  TopologicalManifold.chart: Add description of argument coord_restrictions

comment:32 Changed 3 months ago by
 Commit changed from cdf20b04b24497e66279b5b745f4ef1ca9e7b555 to 741fd2e44276baaad155e2af6b9f2e7b4c1d9b82
Branch pushed to git repo; I updated commit sha1. New commits:
741fd2e  TopologicalManifold.chart: Add an example of using coord_restrictions

comment:33 in reply to: ↑ 30 ; followup: ↓ 34 Changed 3 months ago by
Replying to egourgoulhon:
Replying to mkoeppe:
Replying to egourgoulhon:
 In the INPUT section of the docstring of class
Chart
:
 the argument
coordinates
is described as having a default value, which is no longer true[...]
 the argument
names
should removedActually,
coordinates
andnames
still have the same behavior as before  it is implemented by__classcall__
Yes I understand this; the question is rather about the consistency of the INPUT field with the class declaration as it appears in the reference manual:
class sage.manifolds.chart.Chart(domain, coordinates, calc_method, periods=None, coord_restrictions=None)This is because Sphinx constructs the above from the
__init__
method, not taking into account__classcall__
. If one looks at the html output of the reference manual, the INPUT field arrives just after the class declaration. But I agree with you that this does not reflect the way the class should be invoked. I don't know what is the policy here. Possibly, this issue appears in other parts of Sage as well.
Great point; I have opened #32163 for this general issue.
comment:34 in reply to: ↑ 33 Changed 3 months ago by
comment:35 Changed 3 months ago by
 Reviewers set to Eric Gourgoulhon
 Status changed from needs_review to positive_review
Thanks for this improvement!
comment:36 Changed 3 months ago by
Thanks for reviewing!
comment:37 Changed 3 months ago by
 Commit changed from 741fd2e44276baaad155e2af6b9f2e7b4c1d9b82 to 141ccb5427686ca042815c19d9dc01d3a11ca873
 Status changed from positive_review to needs_review
Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. Last 10 new commits:
0f60232  ConditionSet: Do not sort the conditions, just use _stable_uniq

69d045a  ConditionSet: In doctests, do not rename ZZ^2 etc.

daeb91e  src/sage/sets/set.py: Fix docstring markup

2cf2199  Merge #32015

1eb270a  src/sage/docs/conf.py: Add more \ensuremath to \DeclareUnicodeCharacter

2682469  src/sage/interfaces/sympy_wrapper.py: Use Family, not Set, in doctests to make sure that the SageSet wrapper is actually used

753babb  Set_object_enumerated._sympy_: Translate empty sets to EmptySet

141ecde  Merge #32015

bf62543  Merge branch 't/32089/conditionset__conditionset_callable_symbolic_expression' into t/32009/eliminate_direct_use_of_the_chart__domain_attribute

141ccb5  Merge #32009

comment:38 Changed 3 months ago by
 Status changed from needs_review to positive_review
Merged updated #32009
comment:39 Changed 3 months ago by
 Commit changed from 141ccb5427686ca042815c19d9dc01d3a11ca873 to c218828e5bfea9d5cb75d29cc4e864961397989c
 Status changed from positive_review to needs_review
comment:40 Changed 3 months ago by
 Status changed from needs_review to positive_review
comment:41 Changed 3 months ago by
 Cc vbraun added
 Resolution set to fixed
 Status changed from positive_review to closed
Apparently this has been merged as part of #32089.
I like very much replacements 4 and 5. Using
lambda
function is a good idea! Why is this not immediately applicable? i.e. why #32089 and #32103 have been set as dependencies?