Opened 5 years ago

Closed 5 years ago

# Fix treatment of pullback on parallelizable manifolds

Reported by: Owned by: egourgoulhon major sage-7.6 geometry pullback, manifolds tscrim Eric Gourgoulhon Travis Scrimshaw N/A 02add83 02add83718a340f778b47aacb73a8a61dba2aa18

### Description

The following is a bug:

sage: S1 = Manifold(1, 'S^1') # the circle
sage: U = S1.open_subset('U') # the complement of one point
sage: Xt.<t> =  U.chart('t:(0,2*pi)') # the standard angle coordinate
sage: V = S1.open_subset('V') # the complement of the point t=pi
sage: Xu.<u> = V.chart('u:(0,2*pi)') # the angle t-pi
sage: S1.declare_union(U, V)
sage: e = S1.vector_frame('e') # a global vector frame (makes S^1 parallelizable)
sage: R2 = Manifold(2, 'R^2', start_index=1) # Euclidean space R^2
sage: X2.<x,y> = R2.chart() # Cartesian coordinates
sage: F = S1.diff_map(R2, {(Xt,X2): [cos(t), sin(t)],
....:                      (Xu,X2): [-cos(u), -sin(u)]}) # embedding S^1 --> R^2
sage: dx = X2.coframe()[1] # the 1-form dx on R^2
sage: a = F.pullback(dx) ; a
1-form on the 1-dimensional differentiable manifold S^1
sage: a._components
{Coordinate frame (V, (d/du)): 1-index components w.r.t. Coordinate frame (V, (d/du)),
Coordinate frame (U, (d/dt)): 1-index components w.r.t. Coordinate frame (U, (d/dt))}

As a tensor field on S^1, a should have components only on frames which domain is S^1 and not some strict subsets of it, like U and V. Only restrictions of a to U and V (as stored in a._restrictions) shall be initialized in the current case.

This ticket fixes the issue. It also changes the names of the pullback helper function and its arguments for a better readability. It also corrects some typos in the error messages in methods of DiffMap.

### comment:1 Changed 5 years ago by egourgoulhon

• Status changed from new to needs_review

### comment:2 follow-up: ↓ 3 Changed 5 years ago by tscrim

If _pullback_chart is suppose to be a method, then the first argument should be self to keep with Python conventions. However, if it is better served as a function, then I think we should make it into a proper function.

### comment:3 in reply to: ↑ 2 ; follow-up: ↓ 4 Changed 5 years ago by egourgoulhon

If _pullback_chart is suppose to be a method, then the first argument should be self to keep with Python conventions. However, if it is better served as a function, then I think we should make it into a proper function.

_pullback_chart is not a method; it's only a nested function defined inside the method pullback (because there is no need to call it outside this scope) and its first argument is not the self of pullback. What do you mean by making it a proper function? Removing the first underscore in the name or defining it outside the scope of pullback, or even outside the scope of the class?

Another option would be to turn it into a (private) method, keeping in mind that this method is relevant only for a map whose both domain and codomain are chart domains.

### comment:4 in reply to: ↑ 3 ; follow-up: ↓ 6 Changed 5 years ago by egourgoulhon

If _pullback_chart is suppose to be a method, then the first argument should be self to keep with Python conventions. However, if it is better served as a function, then I think we should make it into a proper function.

_pullback_chart is not a method; it's only a nested function defined inside the method pullback (because there is no need to call it outside this scope) and its first argument is not the self of pullback. What do you mean by making it a proper function? Removing the first underscore in the name or defining it outside the scope of pullback, or even outside the scope of the class?

Another option would be to turn it into a (private) method, keeping in mind that this method is relevant only for a map whose both domain and codomain are chart domains.

Giving a second thought to this, I think I see what you mean: we should consider the nested function _pulback_chart as a method, setting the name of its first argument to self, even if this self is not the self of the enclosing method pull_back (but actually a restriction of it). Correct?

### comment:5 Changed 5 years ago by git

• Commit changed from 96f814daca82e4dfe60a5638f3a54981ca434b5d to 6ab5f636b5db2c2e6a5c7e234e1471ef002e4f13

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

 ​6ab5f63 Change the name of first argument to 'self' in nested method _pullback_chart of DiffMap

### comment:6 in reply to: ↑ 4 Changed 5 years ago by egourgoulhon

Giving a second thought to this, I think I see what you mean: we should consider the nested function _pulback_chart as a method, setting the name of its first argument to self, even if this self is not the self of the enclosing method pull_back (but actually a restriction of it). Correct?

I've applied the change in the above commit.

### comment:7 follow-up: ↓ 9 Changed 5 years ago by tscrim

Oh, I see; was looking at it a little too isolated and didn't notice it was a nested function. So I'm okay with the first arg not being self. However, it does not seem to be using anything in the containing method, so I don't see why it should be nested? We don't have to address it here since this is a separate issue.

Also, this is a further tangent so feel free to ignore (for this ticket), but because jacob is a matrix (at least I think it is), it is (much) faster to do:

+jacob[ind_old[i]-si2,ind_new[i]-si1]
-jacob[ind_old[i]-si2][ind_new[i]-si1]

because it doesn't have to create the intermediate vector object.

### comment:8 Changed 5 years ago by git

• Commit changed from 6ab5f636b5db2c2e6a5c7e234e1471ef002e4f13 to 02add83718a340f778b47aacb73a8a61dba2aa18

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

 ​02add83 Revert to previous version of nested function _pullback_chart in DiffMap.pullback

### comment:9 in reply to: ↑ 7 ; follow-up: ↓ 10 Changed 5 years ago by egourgoulhon

Oh, I see; was looking at it a little too isolated and didn't notice it was a nested function. So I'm okay with the first arg not being self.

OK; I've reverted to the first arg being diff_map in the above commit.

However, it does not seem to be using anything in the containing method, so I don't see why it should be nested?

Well, I made it nested because the only use case of this function is within the method pullback and I did not want to pollute the TAB completion of DiffMap objects.

We don't have to address it here since this is a separate issue.

Also, this is a further tangent so feel free to ignore (for this ticket), but because jacob is a matrix (at least I think it is), it is (much) faster to do:

+jacob[ind_old[i]-si2,ind_new[i]-si1]
-jacob[ind_old[i]-si2][ind_new[i]-si1]

because it doesn't have to create the intermediate vector object.

Thanks for pointing this! I've taken the opportunity of the above commit to correct it.

### comment:10 in reply to: ↑ 9 ; follow-up: ↓ 11 Changed 5 years ago by tscrim

• Reviewers set to Travis Scrimshaw
• Status changed from needs_review to positive_review

However, it does not seem to be using anything in the containing method, so I don't see why it should be nested?

Well, I made it nested because the only use case of this function is within the method pullback and I did not want to pollute the TAB completion of DiffMap objects.

Fair enough. Positive review. Thank you.

### comment:11 in reply to: ↑ 10 Changed 5 years ago by egourgoulhon

Thank you very much for the review!

### comment:12 Changed 5 years ago by vbraun

• Branch changed from public/manifolds/bug-pullback-parallelizable to 02add83718a340f778b47aacb73a8a61dba2aa18
• Resolution set to fixed
• Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.