#32102 closed enhancement (fixed)

Chart: Add init argument coord_restrictions, deprecate method add_restrictions

Reported by: Matthias Köppe Owned by:
Priority: major Milestone: sage-9.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:

Status badges

Description (last modified by Matthias Köppe)

(from https://trac.sagemath.org/ticket/31901#comment:22)

This will remove the appearance of mutability of Charts.

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 Matthias Köppe

Dependencies: #32089

comment:2 Changed 17 months ago by Matthias Köppe

Description: modified (diff)

comment:3 Changed 17 months ago by Matthias Köppe

Dependencies: #32089#32089, #32103

comment:4 Changed 17 months ago by Eric Gourgoulhon

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?

comment:5 Changed 17 months ago by Matthias Köppe

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 Matthias Köppe

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 Charts.

comment:7 Changed 17 months ago by Matthias Köppe

Branch: u/mkoeppe/chart__add_init_argument_coord_restrictions__deprecate_method_add_restrictions

comment:8 Changed 17 months ago by Matthias Köppe

Commit: d9520ae9989dfdccefdf3f20f45389cb4f632dc5

In the branch of #31894 I have a change from def _init_coordinates to @classmethod def _parse_coordinates that would be useful to implement __classcall__.


New commits:

d9520aeChart: Add coord_restrictions init arg

comment:9 in reply to:  8 Changed 17 months ago by Matthias Köppe

Dependencies: #32089, #32103#32116, #32089, #32103

Replying to mkoeppe:

In the branch of #31894 I have a change from def _init_coordinates to @classmethod def _parse_coordinates that would be useful to implement __classcall__.

That's now on #32116.

comment:10 Changed 17 months ago by git

Commit: d9520ae9989dfdccefdf3f20f45389cb4f632dc5ce765863d3c5d81a6460e5e2f7fd305d690337fb

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

8ba174cEliminate direct use of Chart._domain
21297f3Merge #32009
deace83Chart: Replace _init_coordinates by _parse_coordinates
4db4995Chart: Fix up __classcall__ and _parse_coordinates by avoiding unhashable things
fc59c9dChart.__classcall__: Add doctest
ce76586Merge #32116

comment:11 Changed 17 months ago by git

Commit: ce765863d3c5d81a6460e5e2f7fd305d690337fbc31fb4baf8cefdff7a9864e774941caae697e603

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

c31fb4bChart: Make coord restrictions hashable by using frozenset in place of list

comment:12 Changed 17 months ago by Matthias Köppe

Dependencies: #32116, #32089, #32103#32116, #32103

comment:13 in reply to:  5 Changed 17 months ago by Matthias Köppe

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 a ConditionSet (#32089); but I suppose simpler solutions are possible too

I have implemented a simpler solution

comment:14 Changed 17 months ago by git

Commit: c31fb4baf8cefdff7a9864e774941caae697e60330bc60dffdadae7db53364edd7f650d1a3c24d49

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

91c4c98TopologicalManifold.chart: Add keyword arg coord_restrictions
30bc60dChart: Handle lambda-quoted coord_restrictions

comment:15 Changed 17 months ago by Matthias Köppe

Replacements 1, 2, 3, and 5 should work now. For simplicity, let's skip 4.

comment:16 Changed 17 months ago by Matthias Köppe

Dependencies: #32116, #32103#32116

comment:17 Changed 17 months ago by git

Commit: 30bc60dffdadae7db53364edd7f650d1a3c24d49b5820a9aabf558768caccfcbbfaec0f679cb5d26

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

b5820a9Remove most uses of Chart.add_restrictions in doctests

comment:18 Changed 17 months ago by git

Commit: b5820a9aabf558768caccfcbbfaec0f679cb5d261e482307cd15ce4f40cfdfd1d477db3693fd5ab0

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

1e48230RealChart._tighten_bounds: Factor out from RealChart.add_restrictions, call it from __init__ too

comment:19 Changed 17 months ago by git

Commit: 1e482307cd15ce4f40cfdfd1d477db3693fd5ab0939dcd5c3884adad6c757b0ce6cb0840f26b73d6

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

3a5217cChart._normalize_coord_restrictions: Move resolution of lambdas here
4c8e6d3OpenInterval: Remove use of add_restrictions
f8c537dIntegratedCurve: Remove use of add_restrictions in doctests
939dcd5Chart.add_restrictions: Deprecate

comment:20 Changed 17 months ago by Matthias Köppe

Authors: Matthias Koeppe
Cc: Michael Jung Travis Scrimshaw added
Status: newneeds_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 follow-up ticket.

comment:21 Changed 17 months ago by Matthias Köppe

Dependencies: #32116#32116, #32009

comment:22 Changed 17 months ago by git

Commit: 939dcd5c3884adad6c757b0ce6cb0840f26b73d60dfc22e151a7fbcf26d5bb06ce6bc36f5dee366a

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

f43b358Chart._init_coordinates: Fix up use of domain
4e316e9Fix bug in Chart.__init__ regarding the determination of top charts (Trac #32112)
907c9bcMerge #32112
0dfc22eMerge 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 git

Commit: 0dfc22e151a7fbcf26d5bb06ce6bc36f5dee366a0e63ff14b415d468ee43578c512fa48f7813f983

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

1742fdcMerge #32009
0e63ff1Merge #32116

comment:24 Changed 17 months ago by git

Commit: 0e63ff14b415d468ee43578c512fa48f7813f983f66e4bc00b87b87196743273ec8108c3b86fada8

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

9ac1834src/sage/manifolds/chart.py: Add raw string marker
f66e4bcMerge #32116

comment:25 Changed 17 months ago by Matthias Köppe

Description: modified (diff)

comment:26 Changed 17 months ago by Eric Gourgoulhon

Looks good. Here are some remarks:

  1. The argument calc_method of Chart.__init__ has no longer a default value. Is there any reason for this? This is not consistent with RealChart.__init__, which has calc_method=None.
  1. 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 of restrictions should be replaced by coord_restrictions
  • the argument names should removed

Moreover, the arguments should be ordered in the same way as they appear in Chart.__init__.

  1. 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
  1. In the INPUT section of the docstring of TopologicalManifold.chart, the argument coord_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 typing M.chart?.

comment:27 Changed 17 months ago by git

Commit: f66e4bc00b87b87196743273ec8108c3b86fada87c003dba37f63144d36ec37109deb5b9dadeb9eb

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

7c003dbChart.__init__: Restore default of calc_method argument for consistency

comment:28 in reply to:  26 ; Changed 17 months ago by Matthias Köppe

Replying to egourgoulhon:

  1. 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 git

Commit: 7c003dba37f63144d36ec37109deb5b9dadeb9eba39e6fcd968af4a05c777357cdd470e61c244114

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

f85e710Chart: in the description of the argument coord_restrictions, replace all instances of 'restrictions' by 'coord_restrictions'
80f6195Chart, RealChart: In class docstring, order arguments as they appear in __classcall__/__init__
a39e6fcDiffChart, RealDiffChart: In class docstring, order arguments as they appear in __classcall__/__init__; add description of argument coord_restrictions

comment:30 in reply to:  28 ; Changed 17 months ago by Eric Gourgoulhon

Replying to mkoeppe:

Replying to egourgoulhon:

  1. 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__

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:

f85e710Chart: in the description of the argument coord_restrictions, replace all instances of 'restrictions' by 'coord_restrictions'
80f6195Chart, RealChart: In class docstring, order arguments as they appear in __classcall__/__init__
a39e6fcDiffChart, 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 git

Commit: a39e6fcd968af4a05c777357cdd470e61c244114cdf20b04b24497e66279b5b745f4ef1ca9e7b555

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

cdf20b0TopologicalManifold.chart: Add description of argument coord_restrictions

comment:32 Changed 17 months ago by git

Commit: cdf20b04b24497e66279b5b745f4ef1ca9e7b555741fd2e44276baaad155e2af6b9f2e7b4c1d9b82

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

741fd2eTopologicalManifold.chart: Add an example of using coord_restrictions

comment:33 in reply to:  30 ; Changed 17 months ago by Matthias Köppe

Replying to egourgoulhon:

Replying to mkoeppe:

Replying to egourgoulhon:

  1. 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__

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 17 months ago by Eric Gourgoulhon

Replying to mkoeppe:

Great point; I have opened #32163 for this general issue.

OK thanks. Meanwhile, let us leave things as they are, i.e. have the INPUT section of Chart and RealChart docstrings describe __classcall__.

comment:35 Changed 17 months ago by Eric Gourgoulhon

Reviewers: Eric Gourgoulhon
Status: needs_reviewpositive_review

Thanks for this improvement!

comment:36 Changed 17 months ago by Matthias Köppe

Thanks for reviewing!

comment:37 Changed 17 months ago by git

Commit: 741fd2e44276baaad155e2af6b9f2e7b4c1d9b82141ccb5427686ca042815c19d9dc01d3a11ca873
Status: positive_reviewneeds_review

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. Last 10 new commits:

0f60232ConditionSet: Do not sort the conditions, just use _stable_uniq
69d045aConditionSet: In doctests, do not rename ZZ^2 etc.
daeb91esrc/sage/sets/set.py: Fix docstring markup
2cf2199Merge #32015
1eb270asrc/sage/docs/conf.py: Add more \ensuremath to \DeclareUnicodeCharacter
2682469src/sage/interfaces/sympy_wrapper.py: Use Family, not Set, in doctests to make sure that the SageSet wrapper is actually used
753babbSet_object_enumerated._sympy_: Translate empty sets to EmptySet
141ecdeMerge #32015
bf62543Merge branch 't/32089/conditionset__conditionset_callable_symbolic_expression' into t/32009/eliminate_direct_use_of_the_chart__domain_attribute
141ccb5Merge #32009

comment:38 Changed 17 months ago by Matthias Köppe

Status: needs_reviewpositive_review

Merged updated #32009

comment:39 Changed 17 months ago by git

Commit: 141ccb5427686ca042815c19d9dc01d3a11ca873c218828e5bfea9d5cb75d29cc4e864961397989c
Status: positive_reviewneeds_review

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

ea08261Merge #32009
3d1c44dMerge tag '9.4.beta5' into t/32116/chart__parse_coordinates
c218828Merge #32116

comment:40 Changed 17 months ago by Matthias Köppe

Status: needs_reviewpositive_review

comment:41 Changed 17 months ago by Matthias Köppe

Cc: Volker Braun added
Resolution: fixed
Status: positive_reviewclosed

Apparently this has been merged as part of #32089.

Note: See TracTickets for help on using tickets.