Chart: Add init argument coord_restrictions, deprecate method add_restrictions #32102

mkoeppe · 2021-07-02T20:38:42Z

(from #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)

Depends on #32116
Depends on #32009

CC: @egourgoulhon @mjungmath @tscrim @vbraun

Component: manifolds

Author: Matthias Koeppe

Branch/Commit: u/mkoeppe/chart__add_init_argument_coord_restrictions__deprecate_method_add_restrictions @ c218828

Reviewer: Eric Gourgoulhon

Issue created by migration from https://trac.sagemath.org/ticket/32102

The text was updated successfully, but these errors were encountered:

mkoeppe · 2021-07-03T01:21:29Z

Dependencies: #32089

mkoeppe · 2021-07-03T06:08:16Z

Changed dependencies from #32089 to #32089, #32103

egourgoulhon · 2021-07-03T11:14:30Z

comment:4

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?

mkoeppe · 2021-07-03T16:10:51Z

comment:5

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

mkoeppe · 2021-07-03T17:16:21Z

comment:6

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.

mkoeppe · 2021-07-03T17:58:47Z

Branch: u/mkoeppe/chart__add_init_argument_coord_restrictions__deprecate_method_add_restrictions

mkoeppe · 2021-07-03T18:39:20Z

Commit: d9520ae

mkoeppe · 2021-07-03T18:39:20Z

comment:8

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:

`d9520ae`	`Chart: Add coord_restrictions init arg`

mkoeppe · 2021-07-03T20:02:48Z

comment:9

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.

mkoeppe · 2021-07-03T20:02:48Z

Changed dependencies from #32089, #32103 to #32116, #32089, #32103

sagetrac-git · 2021-07-03T20:21:52Z

Changed commit from d9520ae to ce76586

sagetrac-git · 2021-07-03T20:21:52Z

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`

sagetrac-git · 2021-07-03T20:54:42Z

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

`c31fb4b`	`Chart: Make coord restrictions hashable by using frozenset in place of list`

sagetrac-git · 2021-07-03T20:54:42Z

Changed commit from ce76586 to c31fb4b

mkoeppe · 2021-07-03T20:55:21Z

Changed dependencies from #32116, #32089, #32103 to #32116, #32103

mkoeppe · 2021-07-03T20:55:46Z

comment:13

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

sagetrac-git · 2021-07-03T21:28:16Z

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

`91c4c98`	`TopologicalManifold.chart: Add keyword arg coord_restrictions`
`30bc60d`	`Chart: Handle lambda-quoted coord_restrictions`

sagetrac-git · 2021-07-03T21:28:16Z

Changed commit from c31fb4b to 30bc60d

mkoeppe · 2021-07-03T21:29:29Z

comment:15

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

mkoeppe · 2021-07-03T21:29:47Z

Changed dependencies from #32116, #32103 to #32116

sagetrac-git · 2021-07-03T22:00:29Z

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

`b5820a9`	`Remove most uses of Chart.add_restrictions in doctests`

sagetrac-git · 2021-07-03T22:00:29Z

Changed commit from 30bc60d to b5820a9

sagetrac-git · 2021-07-03T22:48:57Z

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

sagetrac-git · 2021-07-03T22:48:57Z

Changed commit from b5820a9 to 1e48230

sagetrac-git · 2021-07-03T23:47:55Z

Changed commit from 1e48230 to 939dcd5

sagetrac-git · 2021-07-08T15:08:30Z

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`

egourgoulhon · 2021-07-08T15:11:43Z

comment:30

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

`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`

sagetrac-git · 2021-07-08T15:14:15Z

Changed commit from a39e6fc to cdf20b0

sagetrac-git · 2021-07-08T15:14:15Z

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

`cdf20b0`	`TopologicalManifold.chart: Add description of argument coord_restrictions`

sagetrac-git · 2021-07-08T15:29:49Z

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

`741fd2e`	`TopologicalManifold.chart: Add an example of using coord_restrictions`

sagetrac-git · 2021-07-08T15:29:49Z

Changed commit from cdf20b0 to 741fd2e

mkoeppe · 2021-07-08T15:37:55Z

comment:33

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

egourgoulhon · 2021-07-09T15:09:29Z

comment:34

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

egourgoulhon · 2021-07-09T15:10:51Z

Reviewer: Eric Gourgoulhon

egourgoulhon · 2021-07-09T15:10:51Z

comment:35

Thanks for this improvement!

mkoeppe · 2021-07-09T15:50:20Z

comment:36

Thanks for reviewing!

sagetrac-git · 2021-07-12T21:24:50Z

Changed commit from 741fd2e to 141ccb5

sagetrac-git · 2021-07-12T21:24:50Z

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`

mkoeppe · 2021-07-12T21:25:16Z

comment:38

Merged updated #32009

sagetrac-git · 2021-07-19T01:54:59Z

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

`ea08261`	`Merge #32009`
`3d1c44d`	`Merge tag '9.4.beta5' into t/32116/chart__parse_coordinates`
`c218828`	`Merge #32116`

sagetrac-git · 2021-07-19T01:54:59Z

Changed commit from 141ccb5 to c218828

mkoeppe · 2021-07-24T19:06:00Z

comment:41

Apparently this has been merged as part of #32089.

mkoeppe added this to the sage-9.4 milestone Jul 2, 2021

mkoeppe added c: manifolds labels Jul 2, 2021

This comment has been minimized.

Sign in to view

egourgoulhon added s: positive review and removed s: needs review labels Jul 9, 2021

sagetrac-git mannequin added s: needs review and removed s: positive review labels Jul 12, 2021

mkoeppe added s: positive review and removed s: needs review labels Jul 12, 2021

sagetrac-git mannequin added s: needs review and removed s: positive review labels Jul 19, 2021

mkoeppe added s: positive review and removed s: needs review labels Jul 19, 2021

mkoeppe removed the s: positive review label Jul 24, 2021

mkoeppe closed this as completed Jul 24, 2021

egourgoulhon mentioned this issue Jan 30, 2023

SageManifolds metaticket 2 #30525

Open

Chart: Add init argument coord_restrictions, deprecate method add_restrictions #32102

Chart: Add init argument coord_restrictions, deprecate method add_restrictions #32102

Comments

mkoeppe commented Jul 2, 2021

mkoeppe commented Jul 3, 2021

This comment has been minimized.

mkoeppe commented Jul 3, 2021

egourgoulhon commented Jul 3, 2021

mkoeppe commented Jul 3, 2021

mkoeppe commented Jul 3, 2021

mkoeppe commented Jul 3, 2021

mkoeppe commented Jul 3, 2021

mkoeppe commented Jul 3, 2021

mkoeppe commented Jul 3, 2021

mkoeppe commented Jul 3, 2021

sagetrac-git mannequin commented Jul 3, 2021

sagetrac-git mannequin commented Jul 3, 2021

sagetrac-git mannequin commented Jul 3, 2021

sagetrac-git mannequin commented Jul 3, 2021

mkoeppe commented Jul 3, 2021

mkoeppe commented Jul 3, 2021

sagetrac-git mannequin commented Jul 3, 2021

sagetrac-git mannequin commented Jul 3, 2021

mkoeppe commented Jul 3, 2021

mkoeppe commented Jul 3, 2021

sagetrac-git mannequin commented Jul 3, 2021

sagetrac-git mannequin commented Jul 3, 2021

sagetrac-git mannequin commented Jul 3, 2021

sagetrac-git mannequin commented Jul 3, 2021

sagetrac-git mannequin commented Jul 3, 2021

sagetrac-git mannequin commented Jul 8, 2021

egourgoulhon commented Jul 8, 2021

sagetrac-git mannequin commented Jul 8, 2021

sagetrac-git mannequin commented Jul 8, 2021

sagetrac-git mannequin commented Jul 8, 2021

sagetrac-git mannequin commented Jul 8, 2021

mkoeppe commented Jul 8, 2021

egourgoulhon commented Jul 9, 2021

egourgoulhon commented Jul 9, 2021

egourgoulhon commented Jul 9, 2021

mkoeppe commented Jul 9, 2021

sagetrac-git mannequin commented Jul 12, 2021

sagetrac-git mannequin commented Jul 12, 2021

mkoeppe commented Jul 12, 2021

sagetrac-git mannequin commented Jul 19, 2021

sagetrac-git mannequin commented Jul 19, 2021

mkoeppe commented Jul 24, 2021