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

Add method tangent_vector to differentiable manifolds #31609

Closed
egourgoulhon opened this issue Apr 5, 2021 · 25 comments
Closed

Add method tangent_vector to differentiable manifolds #31609

egourgoulhon opened this issue Apr 5, 2021 · 25 comments

Comments

@egourgoulhon
Copy link
Member

Currently, constructing a tangent vector v at some point p of a manifold M requires two steps: first construct the tangent space at p, TpM, and then construct v as an element of TpM. For instance:

sage: E.<x,y> = EuclideanSpace()
sage: p = E((1, 2), name='p')
sage: Tp = E.tangent_space(p)   # step 1
sage: v = Tp((-1, 3)); v        # step 2
Vector at Point p on the Euclidean plane E^2

This ticket endows the class DifferentiableManifold with a method tangent_vector, with vector as an alias, to make it a 1-step process. We can now write:

sage: E.<x,y> = EuclideanSpace()
sage: p = E((1, 2), name='p')
sage: v = E.vector(p, -1, 3); v
Vector at Point p on the Euclidean plane E^2

This feature is motivated by this ask.sagemath question.

CC: @slel

Component: manifolds

Keywords: tangent vector

Author: Eric Gourgoulhon

Branch/Commit: b5d7a8f

Reviewer: Michael Jung

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

@egourgoulhon egourgoulhon added this to the sage-9.3 milestone Apr 5, 2021
@egourgoulhon
Copy link
Member Author

@egourgoulhon
Copy link
Member Author

Commit: 5b22596

@egourgoulhon
Copy link
Member Author

New commits:

5b22596Add method vector() to DifferentiableManifold

@mjungmath
Copy link

comment:2

Sweet! I have just one comment. Since Python 3.6, we have f-strings which are way more convenient and easier to read than format (and also faster). I'd propose to use f-strings more often in the future. For this ticket:

- raise ValueError("{} components must be provided".format(dim))
+ raise ValueError(f"{dim} components must be provided")

@mjungmath
Copy link

comment:3

Here is an explanation also elaborating on more advantages of f-strings over format: https://realpython.com/python-f-strings/.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 5, 2021

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

c4a76e9Use f-string in DifferentialManifold.vector

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 5, 2021

Changed commit from 5b22596 to c4a76e9

@egourgoulhon
Copy link
Member Author

comment:5

Replying to @mjungmath:

Here is an explanation also elaborating on more advantages of f-strings over format: https://realpython.com/python-f-strings/.

Thanks for the tip! The change to f-string is performed in the last commit.

@mjungmath
Copy link

comment:6

Is there a reason why you chose vector over tangent_vector for the method's name? The latter would sound more intuitive to me.

@egourgoulhon
Copy link
Member Author

comment:7

Replying to @mjungmath:

Is there a reason why you chose vector over tangent_vector for the method's name? The latter would sound more intuitive to me.

vector is more adapted to elementary use, like in vector calculus in the Euclidean space, when the user might not know what "tangent space" means. Of course, we can make an alias for tangent_vector if you feel it necessary.

@mjungmath
Copy link

comment:8

Mh, I don't know. Manifolds usually don't constitute of vectors. So a vector method wouldn't make much sense there as it sounds to me more like a method that should be reserved for vector spaces only. I'd advocate to rename vector to tangent_vector because mathematically precise.

This is a neat feature though, and I don't want to block it just because of my pedantry. An alias sounds like a good compromise.

@mjungmath
Copy link

comment:9

Alternatively the Euclidean space, as a special case, can be endowed with an alias vector whereas general (differentiable) manifolds only supposed to have tangent_vector. I think that is the best solution to maintain preciseness and having that alias in the elementary case at the same time.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 6, 2021

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

b5d7a8fRename vector to tangent_vector and make vector an alias to it

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 6, 2021

Changed commit from c4a76e9 to b5d7a8f

@egourgoulhon

This comment has been minimized.

@egourgoulhon
Copy link
Member Author

comment:11

Replying to @mjungmath:

Mh, I don't know. Manifolds usually don't constitute of vectors. So a vector method wouldn't make much sense there as it sounds to me more like a method that should be reserved for vector spaces only. I'd advocate to rename vector to tangent_vector because mathematically precise.

OK, this is done in the last commit.
Note however that saying simply "vector at point p of manifold M" is quite unambiguous and is in line with the terminology "vector field": we do not say "tangent vector field", do we?

This is a neat feature though, and I don't want to block it just because of my pedantry. An alias sounds like a good compromise.

Done. vector is now an alias for tangent_vector.

@egourgoulhon egourgoulhon changed the title Add method vector to differentiable manifolds Add method tangent_vector to differentiable manifolds Apr 6, 2021
@egourgoulhon
Copy link
Member Author

comment:12

Replying to @mjungmath:

Alternatively the Euclidean space, as a special case, can be endowed with an alias vector whereas general (differentiable) manifolds only supposed to have tangent_vector. I think that is the best solution to maintain preciseness and having that alias in the elementary case at the same time.

I oppose to this: we shall not introduce on Euclidean spaces method names that break for more general manifolds, while the functionality is exactly the same.

@slel
Copy link
Member

slel commented Apr 6, 2021

comment:13

How about requiring

v = E.vector(p, (-1, 3))

instead of

v = E.vector(p, -1, 3)

I don't mind either way but there's a choice to make.

@egourgoulhon
Copy link
Member Author

comment:14

Replying to @slel:

How about requiring

v = E.vector(p, (-1, 3))

instead of

v = E.vector(p, -1, 3)

I don't mind either way but there's a choice to make.

There is no choice to make ;-) Both work in the current implementation (cf. the examples in the doctests).

@mjungmath
Copy link

comment:15

Replying to @egourgoulhon:

OK, this is done in the last commit.
Note however that saying simply "vector at point p of manifold M" is quite unambiguous and is in line with the terminology "vector field": we do not say "tangent vector field", do we?

No, we don't say "tangent vector field". But I haven't heard or read the use of "vector" over "tangent vector" either. Historical burden I suppose.

It should be unambiguous though and having both is fine with me. Someone who seeks for (tangent) vectors will find them now.

@mjungmath
Copy link

comment:16

LGTM

@mjungmath
Copy link

Reviewer: Michael Jung

@egourgoulhon
Copy link
Member Author

comment:18

Thanks for the review and suggestions!

@mkoeppe
Copy link
Contributor

mkoeppe commented Apr 7, 2021

comment:19

Sage development has entered the release candidate phase for 9.3. Setting a new milestone for this ticket based on a cursory review.

@mkoeppe mkoeppe modified the milestones: sage-9.3, sage-9.4 Apr 7, 2021
@vbraun
Copy link
Member

vbraun commented Jun 6, 2021

Changed branch from public/manifolds/tangent_vector-31609 to b5d7a8f

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