Opened 17 months ago
Closed 17 months ago
#32102 closed enhancement (fixed)
Chart: Add init argument coord_restrictions, deprecate method add_restrictions
Reported by:  Matthias Köppe  Owned by:  

Priority:  major  Milestone:  sage9.4 
Component:  manifolds  Keywords:  
Cc:  Eric Gourgoulhon, Michael Jung, Travis Scrimshaw, Volker Braun  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 17 months ago by
Dependencies:  → #32089 

comment:2 Changed 17 months ago by
Description:  modified (diff) 

comment:3 Changed 17 months ago by
Dependencies:  #32089 → #32089, #32103 

comment:4 Changed 17 months ago by
comment:5 followup: 13 Changed 17 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 17 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 17 months ago by
Branch:  → u/mkoeppe/chart__add_init_argument_coord_restrictions__deprecate_method_add_restrictions 

comment:8 followup: 9 Changed 17 months ago by
Commit:  → d9520ae9989dfdccefdf3f20f45389cb4f632dc5 

comment:9 Changed 17 months ago by
Dependencies:  #32089, #32103 → #32116, #32089, #32103 

comment:10 Changed 17 months ago by
Commit:  d9520ae9989dfdccefdf3f20f45389cb4f632dc5 → 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 17 months ago by
Commit:  ce765863d3c5d81a6460e5e2f7fd305d690337fb → 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 17 months ago by
Dependencies:  #32116, #32089, #32103 → #32116, #32103 

comment:13 Changed 17 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 17 months ago by
Commit:  c31fb4baf8cefdff7a9864e774941caae697e603 → 30bc60dffdadae7db53364edd7f650d1a3c24d49 

comment:15 Changed 17 months ago by
Replacements 1, 2, 3, and 5 should work now. For simplicity, let's skip 4.
comment:16 Changed 17 months ago by
Dependencies:  #32116, #32103 → #32116 

comment:17 Changed 17 months ago by
Commit:  30bc60dffdadae7db53364edd7f650d1a3c24d49 → 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 17 months ago by
Commit:  b5820a9aabf558768caccfcbbfaec0f679cb5d26 → 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 17 months ago by
Commit:  1e482307cd15ce4f40cfdfd1d477db3693fd5ab0 → 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 17 months ago by
Authors:  → Matthias Koeppe 

Cc:  Michael Jung Travis Scrimshaw added 
Status:  new → 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 17 months ago by
Dependencies:  #32116 → #32116, #32009 

comment:22 Changed 17 months ago by
Commit:  939dcd5c3884adad6c757b0ce6cb0840f26b73d6 → 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 17 months ago by
Commit:  0dfc22e151a7fbcf26d5bb06ce6bc36f5dee366a → 0e63ff14b415d468ee43578c512fa48f7813f983 

comment:24 Changed 17 months ago by
Commit:  0e63ff14b415d468ee43578c512fa48f7813f983 → f66e4bc00b87b87196743273ec8108c3b86fada8 

comment:25 Changed 17 months ago by
Description:  modified (diff) 

comment:26 followup: 28 Changed 17 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 17 months ago by
Commit:  f66e4bc00b87b87196743273ec8108c3b86fada8 → 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 followup: 30 Changed 17 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 17 months ago by
Commit:  7c003dba37f63144d36ec37109deb5b9dadeb9eb → 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 followup: 33 Changed 17 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 17 months ago by
Commit:  a39e6fcd968af4a05c777357cdd470e61c244114 → cdf20b04b24497e66279b5b745f4ef1ca9e7b555 

Branch pushed to git repo; I updated commit sha1. New commits:
cdf20b0  TopologicalManifold.chart: Add description of argument coord_restrictions

comment:32 Changed 17 months ago by
Commit:  cdf20b04b24497e66279b5b745f4ef1ca9e7b555 → 741fd2e44276baaad155e2af6b9f2e7b4c1d9b82 

Branch pushed to git repo; I updated commit sha1. New commits:
741fd2e  TopologicalManifold.chart: Add an example of using coord_restrictions

comment:33 followup: 34 Changed 17 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 Changed 17 months ago by
comment:35 Changed 17 months ago by
Reviewers:  → Eric Gourgoulhon 

Status:  needs_review → positive_review 
Thanks for this improvement!
comment:37 Changed 17 months ago by
Commit:  741fd2e44276baaad155e2af6b9f2e7b4c1d9b82 → 141ccb5427686ca042815c19d9dc01d3a11ca873 

Status:  positive_review → 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:39 Changed 17 months ago by
Commit:  141ccb5427686ca042815c19d9dc01d3a11ca873 → c218828e5bfea9d5cb75d29cc4e864961397989c 

Status:  positive_review → needs_review 
comment:40 Changed 17 months ago by
Status:  needs_review → positive_review 

comment:41 Changed 17 months ago by
Cc:  Volker Braun added 

Resolution:  → fixed 
Status:  positive_review → 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?