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:  sage7.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/bugpullbackparallelizable (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 tpi 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 1form dx on R^2 sage: a = F.pullback(dx) ; a 1form on the 1dimensional differentiable manifold S^1 sage: a._components {Coordinate frame (V, (d/du)): 1index components w.r.t. Coordinate frame (V, (d/du)), Coordinate frame (U, (d/dt)): 1index 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
 Status changed from new to needs_review
comment:2 followup: ↓ 3 Changed 13 days ago by
comment:3 in reply to: ↑ 2 ; followup: ↓ 4 Changed 12 days ago by
Replying to tscrim:
If
_pullback_chart
is suppose to be a method, then the first argument should beself
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 ; followup: ↓ 6 Changed 12 days ago by
Replying to egourgoulhon:
Replying to tscrim:
If
_pullback_chart
is suppose to be a method, then the first argument should beself
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 methodpullback
(because there is no need to call it outside this scope) and its first argument is not theself
ofpullback
. What do you mean by making it a proper function? Removing the first underscore in the name or defining it outside the scope ofpullback
, 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
 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 12 days ago by
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 toself
, even if thisself
is not theself
of the enclosing methodpull_back
(but actually a restriction of it). Correct?
I've applied the change in the above commit.
comment:7 followup: ↓ 9 Changed 12 days ago by
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
 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 ; followup: ↓ 10 Changed 11 days ago by
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 ; followup: ↓ 11 Changed 10 days ago by
 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 ofDiffMap
objects.
Fair enough. Positive review. Thank you.
comment:11 in reply to: ↑ 10 Changed 10 days ago by
Thank you very much for the review!
If
_pullback_chart
is suppose to be a method, then the first argument should beself
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.