Opened 7 weeks ago
Closed 4 weeks ago
#31923 closed defect (fixed)
The inverse of the inverse should be self in CoordChange
Reported by: | egourgoulhon | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-9.4 |
Component: | manifolds | Keywords: | chart, transition_map |
Cc: | tscrim, gh-mjungmath, mkoeppe | Merged in: | |
Authors: | Eric Gourgoulhon | Reviewers: | Michael Jung |
Report Upstream: | N/A | Work issues: | |
Branch: | 923196a (Commits, GitHub, GitLab) | Commit: | 923196adb6c2e75ef44c4e9ce4df171cf607585c |
Dependencies: | Stopgaps: |
Description
When initializing the inverse of a transition map between two charts, either by the method CoordChange.inverse
(computation of the inverse) or by CoordChange.set_inverse
(inverse provided by the user), the attribute _inverse
of self
is set to the inverse object (for caching). Obviously, by symmetry, the attribute _inverse
of the inverse should be set to self
, but it is not. As a consequence, we have in Sage 9.3:
sage: E.<x,y> = EuclideanSpace() sage: cart = E.cartesian_coordinates() sage: polar = E.polar_coordinates() sage: polar_to_cart = E.coord_change(polar, cart) sage: polar_to_cart.display() x = r*cos(ph) y = r*sin(ph) sage: cart_to_polar = E.coord_change(cart, polar) sage: cart_to_polar.display() r = sqrt(x^2 + y^2) ph = arctan2(y, x) sage: polar_to_cart.inverse() is cart_to_polar True
So far so good, but
sage: cart_to_polar.inverse() is polar_to_cart ... ValueError: no solution found; use set_inverse() to set the inverse manually
This is fixed in this ticket by the following one-line addition to the code of CoordChange.inverse
and CoordChange.set_inverse
:
self._inverse._inverse = self
Besides, this ticket improves a little bit the documentation of CoordChange.inverse
.
Change History (15)
comment:1 Changed 7 weeks ago by
- Branch set to public/manifolds/transition_map_inverse-31923
- Cc tscrim gh-mjungmath mkoeppe added
- Commit set to 923196adb6c2e75ef44c4e9ce4df171cf607585c
- Status changed from new to needs_review
comment:2 follow-up: ↓ 4 Changed 7 weeks ago by
- Reviewers set to Michael Jung
LGTM.
Green bot => Green light
comment:3 Changed 7 weeks ago by
Thank you for this superfast review!
comment:4 in reply to: ↑ 2 Changed 7 weeks ago by
Replying to gh-mjungmath:
Green bot => Green light
Well, no bot at all... I wonder why no patchbot has visited this ticket after two days. Maybe they are super busy at the moment...
comment:5 follow-up: ↓ 7 Changed 6 weeks ago by
Mh, indeed. :-/
Maybe you can push the changes into another branch again to trigger the patchbot?
comment:6 Changed 6 weeks ago by
- Branch changed from public/manifolds/transition_map_inverse-31923 to public/manifolds/transition_map_inverse_of_inverse-31923
comment:7 in reply to: ↑ 5 Changed 6 weeks ago by
Replying to gh-mjungmath:
Mh, indeed. :-/
Maybe you can push the changes into another branch again to trigger the patchbot?
Done. Let's see...
comment:8 follow-up: ↓ 10 Changed 6 weeks ago by
IMO, that won't change anything.
comment:9 Changed 6 weeks ago by
The ticket appears now at the top of the list at https://patchbot.sagemath.org/, but the order in this list is probably just the last modification time and not the priority for patchbots.
comment:10 in reply to: ↑ 8 Changed 6 weeks ago by
comment:11 Changed 6 weeks ago by
Patchbots seem to be having a hard time with #31928, see ticket:31928#comment:1.
comment:12 Changed 6 weeks ago by
The patchbot came eventually and is morally green: it reports only one doctest failure in src/sage/misc/package.py
, which is the issue mentioned by Samuel in comment:11 and is not connected with the current ticket.
comment:13 follow-up: ↓ 14 Changed 5 weeks ago by
- Status changed from needs_review to positive_review
On behalf of Michael, in view of comment:4 and comment:12.
comment:14 in reply to: ↑ 13 Changed 5 weeks ago by
comment:15 Changed 4 weeks ago by
- Branch changed from public/manifolds/transition_map_inverse_of_inverse-31923 to 923196adb6c2e75ef44c4e9ce4df171cf607585c
- Resolution set to fixed
- Status changed from positive_review to closed
New commits:
#31923: initialize the inverse of the inverse to self