Opened 2 weeks ago

Last modified 10 days ago

#22563 positive_review defect

Fix treatment of pullback on parallelizable manifolds

Reported by: egourgoulhon Owned by:
Priority: major Milestone: sage-7.6
Component: geometry Keywords: pullback, manifolds
Cc: tscrim Merged in:
Authors: Eric Gourgoulhon Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: public/manifolds/bug-pullback-parallelizable (Commits) Commit: 02add83718a340f778b47aacb73a8a61dba2aa18
Dependencies: Stopgaps:

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.

Change History (11)

comment:1 Changed 2 weeks ago by egourgoulhon

  • Status changed from new to needs_review

comment:2 follow-up: Changed 13 days 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: Changed 12 days ago by 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.

comment:4 in reply to: ↑ 3 ; follow-up: Changed 12 days ago by 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?

comment:5 Changed 12 days ago by git

  • Commit changed from 96f814daca82e4dfe60a5638f3a54981ca434b5d to 6ab5f636b5db2c2e6a5c7e234e1471ef002e4f13

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

6ab5f63Change the name of first argument to 'self' in nested method _pullback_chart of DiffMap

comment:6 in reply to: ↑ 4 Changed 12 days ago by 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.

comment:7 follow-up: Changed 12 days 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 11 days ago by git

  • Commit changed from 6ab5f636b5db2c2e6a5c7e234e1471ef002e4f13 to 02add83718a340f778b47aacb73a8a61dba2aa18

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

02add83Revert to previous version of nested function _pullback_chart in DiffMap.pullback

comment:9 in reply to: ↑ 7 ; follow-up: Changed 11 days ago by 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.

comment:10 in reply to: ↑ 9 ; follow-up: Changed 10 days ago by tscrim

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

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.

comment:11 in reply to: ↑ 10 Changed 10 days ago by egourgoulhon

Thank you very much for the review!

Note: See TracTickets for help on using tickets.