Opened 6 months ago

Closed 5 months ago

#32929 closed defect (fixed)

Bad determination of the coordinate range when restricting charts to subdomains

Reported by: egourgoulhon Owned by:
Priority: critical Milestone: sage-9.5
Component: manifolds Keywords: chart
Cc: mkoeppe, tscrim, gh-mjungmath 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 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__.

Change History (12)

comment:1 Changed 6 months ago by egourgoulhon

  • Cc mkoeppe added

comment:2 Changed 6 months ago by egourgoulhon

  • Branch set to public/manifolds/bug_chart_restrict-32929
  • Commit set to 3ad54075a77a5c2ad64204a20842a90a8f8c3e83
  • Description modified (diff)
  • Keywords chart added
  • Priority changed from major to critical
  • Status changed from new to needs_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 6 months ago by egourgoulhon

  • Authors set to Eric Gourgoulhon

comment:4 Changed 6 months ago by egourgoulhon

  • Cc tscrim gh-mjungmath added

comment:5 follow-up: Changed 6 months ago by tscrim

  • Reviewers set to 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 6 months ago by git

  • Commit changed from 3ad54075a77a5c2ad64204a20842a90a8f8c3e83 to 33bdc1c4d8a11ee1ccacc7738845a9d0d45d11e1

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

33bdc1cFix docstring formatting in #32929

comment:7 in reply to: ↑ 5 Changed 6 months ago by egourgoulhon

Replying to tscrim:

Thank you for the review!

Just a trivial change for doc formatting:

Corrected in the above commit.

comment:8 follow-up: Changed 6 months ago by tscrim

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

comment:9 Changed 6 months ago by git

  • Commit changed from 33bdc1c4d8a11ee1ccacc7738845a9d0d45d11e1 to b5f7af599a70276566a0c32c1ee804d46e47954d

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

b5f7af5Fix documentation for #32929

comment:10 in reply to: ↑ 8 Changed 6 months ago by egourgoulhon

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

  • Status changed from needs_review to positive_review

in view of comment:5.

Thank you Travis!

comment:12 Changed 5 months ago by vbraun

  • Branch changed from public/manifolds/bug_chart_restrict-32929 to b5f7af599a70276566a0c32c1ee804d46e47954d
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.