Fix treatment of pullback on parallelizable manifolds

[egourgoulhon]

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.

[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.

[egourgoulhon]

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

_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.

[egourgoulhon]

Replying to egourgoulhon:

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

_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?

[git]

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

[egourgoulhon]

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

[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.

[git]

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

​02add83

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

[egourgoulhon]

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

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.

[tscrim]

Replying to egourgoulhon:

Replying to tscrim:

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.

[egourgoulhon]

Thank you very much for the review!