Bad determination of the coordinate range when restricting charts to subdomains

[egourgoulhon]

Since Sage 9.4, we have

sage: M = Manifold(2, 'M')
sage: X.<x,y> = M.chart(r"x:(0,+oo) y:(0,2):periodic")
sage: X.coord_range()
x: (0, +oo); y: [0, 2] (periodic)
sage: U = M.open_subset('U', coord_def={X: x<1})
sage: X.restrict(U).coord_range()
x: (-oo, 1); y: (-oo, +oo)

The lower bound for x should be O, not -oo, and y should appear as a periodic coordinate, i.e. one should get

sage: X.restrict(U).coord_range()
x: (0, 1); y: [0, 2] (periodic)

Sage <= 9.3 was free of this bug. In Sage >= 9.4, one can trace it to the optional argument bounds of RealChart.__init__, which is used in RealChart.restrict (cf. the line res = type(self)(..., bounds=self._bounds, ...))
and which is not correctly transmitted by Chart.__classcall__.

CC: @mkoeppe @tscrim @mjungmath

Component: manifolds

Keywords: chart

Author: Eric Gourgoulhon

Branch/Commit: b5f7af5

Reviewer: Travis Scrimshaw

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

[egourgoulhon]

comment:2

Here is a proposed fix. The bug was triggered in Chart.__classcall__ by the unconditional resetting of the argument coordinate_options, so that neither bounds nor periods could be transmitted to __init__ while constructing the restricted chart in Chart.restrict or RealChart.restrict.

In correcting the bug, I had to change the attribute _periods of charts from a dictionary to a tuple, to make it hashable. Hence the changes in the file point.py (method ManifoldPoint.__eq__). As a benefit, the output of Chart.periods() is more readable.

The doctest change in line 669 of manifold.py simply restores the correct coordinate values for a point constructed via TopologicalManifold._an_element_(). Indeed, git blame reveals that this doctest was incorrectly changed when the bug was introduced in Sage 9.4.

---

New commits:

3ad5407

Fix bug in chart restrictions to subdomain (#32929)

[egourgoulhon]

Branch: public/manifolds/bug_chart_restrict-32929

[egourgoulhon]

Commit: 3ad5407

[egourgoulhon]

Changed keywords from none to chart

[egourgoulhon]

Author: Eric Gourgoulhon

[tscrim]

comment:5

Just a trivial change for doc formatting:

         - a tuple of variables (as elements of ``SR``)
         - a dictionary with possible keys:
-          - `"periods"`: a tuple of periods
-          - `"bounds"`: a tuple of coordinate ranges
+
+          * ``"periods"``: a tuple of periods
+          * ``"bounds"``: a tuple of coordinate ranges

Once changed, you can set a positive review.

[tscrim]

Reviewer: Travis Scrimshaw

[sagetrac-git]

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

33bdc1c

Fix docstring formatting in #32929

[sagetrac-git]

Changed commit from 3ad5407 to 33bdc1c

[egourgoulhon]

comment:7

Replying to @tscrim:

Thank you for the review!

Just a trivial change for doc formatting:

Corrected in the above commit.

[tscrim]

comment:8

The other thing that is needed (I believe) in the blank line between the indented bullets.

[sagetrac-git]

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

b5f7af5

Fix documentation for #32929

[sagetrac-git]

Changed commit from 33bdc1c to b5f7af5

[egourgoulhon]

comment:10

Replying to @tscrim:

The other thing that is needed (I believe) in the blank line between the indented bullets.

Thanks for catching this. This is corrected in the last commit.

[egourgoulhon]

comment:11

in view of comment:5.

Thank you Travis!

[vbraun]

Changed branch from public/manifolds/bug_chart_restrict-32929 to b5f7af5