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:  sage9.5 
Component:  manifolds  Keywords:  chart 
Cc:  mkoeppe, tscrim, ghmjungmath  Merged in:  
Authors:  Eric Gourgoulhon  Reviewers:  Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  b5f7af5 (Commits, GitHub, GitLab)  Commit:  b5f7af599a70276566a0c32c1ee804d46e47954d 
Dependencies:  Stopgaps: 
Description (last modified by )
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
 Cc mkoeppe added
comment:2 Changed 6 months ago by
 Branch set to public/manifolds/bug_chart_restrict32929
 Commit set to 3ad54075a77a5c2ad64204a20842a90a8f8c3e83
 Description modified (diff)
 Keywords chart added
 Priority changed from major to critical
 Status changed from new to needs_review
comment:3 Changed 6 months ago by
comment:4 Changed 6 months ago by
 Cc tscrim ghmjungmath added
comment:5 followup: ↓ 7 Changed 6 months ago by
 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
 Commit changed from 3ad54075a77a5c2ad64204a20842a90a8f8c3e83 to 33bdc1c4d8a11ee1ccacc7738845a9d0d45d11e1
Branch pushed to git repo; I updated commit sha1. New commits:
33bdc1c  Fix docstring formatting in #32929

comment:7 in reply to: ↑ 5 Changed 6 months ago by
Replying to tscrim:
Thank you for the review!
Just a trivial change for doc formatting:
Corrected in the above commit.
comment:8 followup: ↓ 10 Changed 6 months ago by
The other thing that is needed (I believe) in the blank line between the indented bullets.
comment:9 Changed 6 months ago by
 Commit changed from 33bdc1c4d8a11ee1ccacc7738845a9d0d45d11e1 to b5f7af599a70276566a0c32c1ee804d46e47954d
Branch pushed to git repo; I updated commit sha1. New commits:
b5f7af5  Fix documentation for #32929

comment:10 in reply to: ↑ 8 Changed 6 months ago by
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
 Status changed from needs_review to positive_review
in view of comment:5.
Thank you Travis!
comment:12 Changed 5 months ago by
 Branch changed from public/manifolds/bug_chart_restrict32929 to b5f7af599a70276566a0c32c1ee804d46e47954d
 Resolution set to fixed
 Status changed from positive_review to closed
Here is a proposed fix. The bug was triggered in
Chart.__classcall__
by the unconditional resetting of the argumentcoordinate_options
, so that neitherbounds
norperiods
could be transmitted to__init__
while constructing the restricted chart inChart.restrict
orRealChart.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 filepoint.py
(methodManifoldPoint.__eq__
). As a benefit, the output ofChart.periods()
is more readable.The doctest change in line 669 of
manifold.py
simply restores the correct coordinate values for a point constructed viaTopologicalManifold._an_element_()
. Indeed,git blame
reveals that this doctest was incorrectly changed when the bug was introduced in Sage 9.4.New commits:
Fix bug in chart restrictions to subdomain (#32929)