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: sage-9.4
Component: manifolds Keywords:
Cc: egourgoulhon, gh-mjungmath, 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:

Status badges

Description (last modified by mkoeppe)

(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 4 months ago by mkoeppe

  • Dependencies set to #32089

comment:2 Changed 4 months ago by mkoeppe

  • Description modified (diff)

comment:3 Changed 4 months ago by mkoeppe

  • Dependencies changed from #32089 to #32089, #32103

comment:4 Changed 4 months ago by egourgoulhon

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 follow-up: Changed 4 months ago by mkoeppe

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 mkoeppe

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 4 months ago by mkoeppe

  • Branch set to u/mkoeppe/chart__add_init_argument_coord_restrictions__deprecate_method_add_restrictions

comment:8 follow-up: Changed 4 months ago by mkoeppe

  • Commit set to 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 4 months ago by mkoeppe

  • Dependencies changed from #32089, #32103 to #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 4 months ago by git

  • Commit changed from d9520ae9989dfdccefdf3f20f45389cb4f632dc5 to ce765863d3c5d81a6460e5e2f7fd305d690337fb

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 4 months ago by git

  • Commit changed from ce765863d3c5d81a6460e5e2f7fd305d690337fb to c31fb4baf8cefdff7a9864e774941caae697e603

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 4 months ago by mkoeppe

  • Dependencies changed from #32116, #32089, #32103 to #32116, #32103

comment:13 in reply to: ↑ 5 Changed 4 months ago by mkoeppe

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 4 months ago by git

  • Commit changed from c31fb4baf8cefdff7a9864e774941caae697e603 to 30bc60dffdadae7db53364edd7f650d1a3c24d49

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 4 months ago by mkoeppe

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

comment:16 Changed 4 months ago by mkoeppe

  • Dependencies changed from #32116, #32103 to #32116

comment:17 Changed 4 months ago by git

  • Commit changed from 30bc60dffdadae7db53364edd7f650d1a3c24d49 to b5820a9aabf558768caccfcbbfaec0f679cb5d26

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

b5820a9Remove most uses of Chart.add_restrictions in doctests

comment:18 Changed 4 months ago by git

  • Commit changed from b5820a9aabf558768caccfcbbfaec0f679cb5d26 to 1e482307cd15ce4f40cfdfd1d477db3693fd5ab0

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 4 months ago by git

  • Commit changed from 1e482307cd15ce4f40cfdfd1d477db3693fd5ab0 to 939dcd5c3884adad6c757b0ce6cb0840f26b73d6

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 4 months ago by mkoeppe

  • Authors set to Matthias Koeppe
  • Cc gh-mjungmath 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 follow-up ticket.

comment:21 Changed 4 months ago by mkoeppe

  • Dependencies changed from #32116 to #32116, #32009

comment:22 Changed 4 months ago by git

  • Commit changed from 939dcd5c3884adad6c757b0ce6cb0840f26b73d6 to 0dfc22e151a7fbcf26d5bb06ce6bc36f5dee366a

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 4 months ago by git

  • Commit changed from 0dfc22e151a7fbcf26d5bb06ce6bc36f5dee366a to 0e63ff14b415d468ee43578c512fa48f7813f983

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

1742fdcMerge #32009
0e63ff1Merge #32116

comment:24 Changed 4 months ago by git

  • Commit changed from 0e63ff14b415d468ee43578c512fa48f7813f983 to f66e4bc00b87b87196743273ec8108c3b86fada8

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 4 months ago by mkoeppe

  • Description modified (diff)

comment:26 follow-up: Changed 3 months ago by egourgoulhon

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 3 months ago by git

  • Commit changed from f66e4bc00b87b87196743273ec8108c3b86fada8 to 7c003dba37f63144d36ec37109deb5b9dadeb9eb

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 ; follow-up: Changed 3 months ago by 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__

comment:29 Changed 3 months ago by git

  • Commit changed from 7c003dba37f63144d36ec37109deb5b9dadeb9eb to a39e6fcd968af4a05c777357cdd470e61c244114

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 ; follow-up: Changed 3 months ago by 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.


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 3 months ago by git

  • Commit changed from a39e6fcd968af4a05c777357cdd470e61c244114 to cdf20b04b24497e66279b5b745f4ef1ca9e7b555

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

cdf20b0TopologicalManifold.chart: Add description of argument coord_restrictions

comment:32 Changed 3 months ago by git

  • Commit changed from cdf20b04b24497e66279b5b745f4ef1ca9e7b555 to 741fd2e44276baaad155e2af6b9f2e7b4c1d9b82

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 ; follow-up: Changed 3 months ago by mkoeppe

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 3 months ago by egourgoulhon

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 3 months ago by egourgoulhon

  • Reviewers set to Eric Gourgoulhon
  • Status changed from needs_review to positive_review

Thanks for this improvement!

comment:36 Changed 3 months ago by mkoeppe

Thanks for reviewing!

comment:37 Changed 3 months ago by git

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

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 3 months ago by mkoeppe

  • Status changed from needs_review to positive_review

Merged updated #32009

comment:39 Changed 3 months ago by git

  • Commit changed from 141ccb5427686ca042815c19d9dc01d3a11ca873 to c218828e5bfea9d5cb75d29cc4e864961397989c
  • Status changed from positive_review to needs_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 3 months ago by mkoeppe

  • Status changed from needs_review to positive_review

comment:41 Changed 3 months ago by mkoeppe

  • Cc vbraun added
  • Resolution set to fixed
  • Status changed from positive_review to closed

Apparently this has been merged as part of #32089.

Note: See TracTickets for help on using tickets.