#32929 closed defect (fixed)

Bad determination of the coordinate range when restricting charts to subdomains

Reported by: Eric Gourgoulhon Owned by:
Priority: critical Milestone: sage-9.5
Component: manifolds Keywords: chart
Cc: Matthias Köppe, Travis Scrimshaw, Michael Jung Merged in:
Authors: Eric Gourgoulhon Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: b5f7af5 (Commits, GitHub, GitLab) Commit: b5f7af599a70276566a0c32c1ee804d46e47954d
Dependencies: Stopgaps:

Status badges

Description (last modified by Eric Gourgoulhon)

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

Change History (12)

comment:1 Changed 13 months ago by Eric Gourgoulhon

Cc: Matthias Köppe added

comment:2 Changed 12 months ago by Eric Gourgoulhon

Branch: public/manifolds/bug_chart_restrict-32929
Commit: 3ad54075a77a5c2ad64204a20842a90a8f8c3e83
Description: modified (diff)
Keywords: chart added
Priority: majorcritical
Status: newneeds_review

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:

3ad5407Fix bug in chart restrictions to subdomain (#32929)

comment:3 Changed 12 months ago by Eric Gourgoulhon

Authors: Eric Gourgoulhon

comment:4 Changed 12 months ago by Eric Gourgoulhon

Cc: Travis Scrimshaw Michael Jung added

comment:5 Changed 12 months ago by Travis Scrimshaw

Reviewers: Travis Scrimshaw

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.

comment:6 Changed 12 months ago by git

Commit: 3ad54075a77a5c2ad64204a20842a90a8f8c3e8333bdc1c4d8a11ee1ccacc7738845a9d0d45d11e1

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

33bdc1cFix docstring formatting in #32929

comment:7 in reply to:  5 Changed 12 months ago by Eric Gourgoulhon

Replying to tscrim:

Thank you for the review!

Just a trivial change for doc formatting:

Corrected in the above commit.

comment:8 Changed 12 months ago by Travis Scrimshaw

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

comment:9 Changed 12 months ago by git

Commit: 33bdc1c4d8a11ee1ccacc7738845a9d0d45d11e1b5f7af599a70276566a0c32c1ee804d46e47954d

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

b5f7af5Fix documentation for #32929

comment:10 in reply to:  8 Changed 12 months ago by Eric Gourgoulhon

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.

comment:11 Changed 12 months ago by Eric Gourgoulhon

Status: needs_reviewpositive_review

in view of comment:5.

Thank you Travis!

comment:12 Changed 12 months ago by Volker Braun

Branch: public/manifolds/bug_chart_restrict-32929b5f7af599a70276566a0c32c1ee804d46e47954d
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.