Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The inverse of the inverse should be self in CoordChange #31923

Closed
egourgoulhon opened this issue Jun 7, 2021 · 18 comments
Closed

The inverse of the inverse should be self in CoordChange #31923

egourgoulhon opened this issue Jun 7, 2021 · 18 comments

Comments

@egourgoulhon
Copy link
Member

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.

CC: @tscrim @mjungmath @mkoeppe

Component: manifolds

Keywords: chart, transition_map

Author: Eric Gourgoulhon

Branch/Commit: 923196a

Reviewer: Michael Jung

Issue created by migration from https://trac.sagemath.org/ticket/31923

@egourgoulhon egourgoulhon added this to the sage-9.4 milestone Jun 7, 2021
@egourgoulhon
Copy link
Member Author

Commit: 923196a

@egourgoulhon
Copy link
Member Author

New commits:

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

@egourgoulhon
Copy link
Member Author

@mjungmath
Copy link

comment:2

LGTM.

Green bot => Green light

@mjungmath
Copy link

Reviewer: Michael Jung

@egourgoulhon
Copy link
Member Author

comment:3

Thank you for this superfast review!

@egourgoulhon
Copy link
Member Author

comment:4

Replying to @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...

@mjungmath
Copy link

comment:5

Mh, indeed. :-/

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

@egourgoulhon
Copy link
Member Author

@egourgoulhon
Copy link
Member Author

comment:7

Replying to @mjungmath:

Mh, indeed. :-/

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

Done. Let's see...

@tscrim
Copy link
Collaborator

tscrim commented Jun 10, 2021

comment:8

IMO, that won't change anything.

@egourgoulhon
Copy link
Member Author

comment:9

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.

@egourgoulhon
Copy link
Member Author

comment:10

Replying to @tscrim:

IMO, that won't change anything.

You were right, Travis, as always ;-)

@slel
Copy link
Member

slel commented Jun 11, 2021

comment:11

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

@egourgoulhon
Copy link
Member Author

comment:12

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.

@egourgoulhon
Copy link
Member Author

comment:13

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

@mjungmath
Copy link

comment:14

Replying to @egourgoulhon:

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

thumbs up!

@vbraun
Copy link
Member

vbraun commented Jun 29, 2021

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants