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:

Status badges

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 egourgoulhon

  • 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

New commits:

923196a#31923: initialize the inverse of the inverse to self

comment:2 follow-up: Changed 7 weeks ago by gh-mjungmath

  • Reviewers set to Michael Jung

LGTM.

Green bot => Green light

comment:3 Changed 7 weeks ago by egourgoulhon

Thank you for this superfast review!

comment:4 in reply to: ↑ 2 Changed 7 weeks ago by egourgoulhon

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: Changed 6 weeks ago by gh-mjungmath

Mh, indeed. :-/

Maybe you can push the changes into another branch again to trigger the patchbot?

comment:6 Changed 6 weeks ago by egourgoulhon

  • 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 egourgoulhon

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: Changed 6 weeks ago by tscrim

IMO, that won't change anything.

comment:9 Changed 6 weeks ago by egourgoulhon

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 egourgoulhon

Replying to tscrim:

IMO, that won't change anything.

You were right, Travis, as always ;-)

comment:11 Changed 6 weeks ago by slelievre

Patchbots seem to be having a hard time with #31928, see ticket:31928#comment:1.

comment:12 Changed 6 weeks ago by egourgoulhon

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.

Last edited 6 weeks ago by egourgoulhon (previous) (diff)

comment:13 follow-up: Changed 5 weeks ago by egourgoulhon

  • 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 gh-mjungmath

Replying to egourgoulhon:

On behalf of Michael, in view of comment:4 and comment:12.

thumbs up!

comment:15 Changed 4 weeks ago by vbraun

  • Branch changed from public/manifolds/transition_map_inverse_of_inverse-31923 to 923196adb6c2e75ef44c4e9ce4df171cf607585c
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.