Opened 6 months ago
Closed 4 months ago
#31633 closed enhancement (fixed)
Simplify VectorField.__call__
Reported by:  egourgoulhon  Owned by:  

Priority:  major  Milestone:  sage9.4 
Component:  manifolds  Keywords:  vector field, scalar field 
Cc:  tscrim, ghmjungmath  Merged in:  
Authors:  Eric Gourgoulhon  Reviewers:  Michael Jung 
Report Upstream:  N/A  Work issues:  
Branch:  bc25781 (Commits, GitHub, GitLab)  Commit:  bc257815fcd42a6ba49eb0639ad1dc7e6a172f03 
Dependencies:  Stopgaps: 
Description
The method VectorField.__call__
implements the action v(f) of a vector field v on a scalar field f, as a derivation. It is reimplemented here to simply return df(v), i.e. the differential of f acting on v as a 1form. Actually, the current code of this method contains the comment:
#!# Could it be simply # return scalar.differential()(self) # ?
git blame reveals that this comment dates back to 2015. It is time to make the change, thereby simplifying the code and avoiding some code duplication.
Change History (8)
comment:1 Changed 6 months ago by
 Branch set to public/manifolds/vector_field_call_31633
 Cc tscrim ghmjungmath added
 Commit set to c921e9738ac5d828eec7f8a0b0cbd60eba81a9f0
 Status changed from new to needs_review
comment:2 followup: ↓ 4 Changed 6 months ago by
Nice! It's good to reduce code and get rid of duplication!
Two things:
 What exactly caused the change
sage: isinstance(Tp, FiniteRankFreeModule) True sage: sorted(Tp.bases(), key=str)  [Basis (d/dr,d/dph) on the Tangent space at Point p on the Euclidean plane E^2,  Basis (e_r,e_ph) on the Tangent space at Point p on the Euclidean plane E^2, + [Basis (e_r,e_ph) on the Tangent space at Point p on the Euclidean plane E^2, Basis (e_x,e_y) on the Tangent space at Point p on the Euclidean plane E^2]
 It is possible to combine raw strings and fstrings:
 latex_name = r"{}\left({}\right)".format(self._latex_name,  scalar._latex_name) + latex_name = fr"{self._latex_name}\left({scalar._latex_name}\right)"
comment:3 Changed 6 months ago by
 Commit changed from c921e9738ac5d828eec7f8a0b0cbd60eba81a9f0 to bc257815fcd42a6ba49eb0639ad1dc7e6a172f03
Branch pushed to git repo; I updated commit sha1. New commits:
bc25781  Combine raw string and fstring in VectorField.__call__

comment:4 in reply to: ↑ 2 ; followup: ↓ 5 Changed 6 months ago by
Replying to ghmjungmath:
Thanks for your comments.
 What exactly caused the change
sage: isinstance(Tp, FiniteRankFreeModule) True sage: sorted(Tp.bases(), key=str)  [Basis (d/dr,d/dph) on the Tangent space at Point p on the Euclidean plane E^2,  Basis (e_r,e_ph) on the Tangent space at Point p on the Euclidean plane E^2, + [Basis (e_r,e_ph) on the Tangent space at Point p on the Euclidean plane E^2, Basis (e_x,e_y) on the Tangent space at Point p on the Euclidean plane E^2]
It's because in the previous VectorFieldParal.__call__
code, the computation was performed as v^{i} df/dx^{i} in all possible coordinate frames d/dx^{i}, thereby triggering the evaluation of v in these frames. In the new code, the computation is performed as (df)_{i} v^{i} in a single frame (and not necessarily a coordinate one). In the specific example above, v(F)
in line 577 of src/doc/en/thematic_tutorials/vector_calculus/vector_calc_plane.rst
triggered the computation of v in the coordinate frame (d/dr, d/dph)
; this was done in line 1779 of the old (i.e. 9.3.rc2) file vectorfield.py
:
self_r.comp(chart._frame)
Then vp = v.at(p)
in line 634 of vector_calc_plane.rst
created the basis (d/dr, d/dph)
in Tp
; this was done by frame.at(point)
in line 2109 of tensorfield_paral.py
:
comp_resu = resu.add_comp(frame.at(point))
This explains why Tp
had the extra basis (d/dr, d/dph)
with the old code. It was a side effect of an unnecessary calculation in the old VectorFieldParal.__call__
.
 It is possible to combine raw strings and fstrings:
 latex_name = r"{}\left({}\right)".format(self._latex_name,  scalar._latex_name) + latex_name = fr"{self._latex_name}\left({scalar._latex_name}\right)"
Done in the last commit.
comment:5 in reply to: ↑ 4 Changed 6 months ago by
Replying to egourgoulhon:
It's because in the previous
VectorFieldParal.__call__
code, the computation was performed as v^{i} df/dx^{i} in all possible coordinate frames d/dx^{i}, thereby triggering the evaluation of v in these frames. In the new code, the computation is performed as (df)_{i} v^{i} in a single frame (and not necessarily a coordinate one). In the specific example above,v(F)
in line 577 ofsrc/doc/en/thematic_tutorials/vector_calculus/vector_calc_plane.rst
triggered the computation of v in the coordinate frame(d/dr, d/dph)
; this was done in line 1779 of the old (i.e. 9.3.rc2) filevectorfield.py
:self_r.comp(chart._frame)Then
vp = v.at(p)
in line 634 ofvector_calc_plane.rst
created the basis(d/dr, d/dph)
inTp
; this was done byframe.at(point)
in line 2109 oftensorfield_paral.py
:comp_resu = resu.add_comp(frame.at(point))This explains why
Tp
had the extra basis(d/dr, d/dph)
with the old code. It was a side effect of an unnecessary calculation in the oldVectorFieldParal.__call__
.
That's indeed an improvement. Sweet!
 It is possible to combine raw strings and fstrings:
 latex_name = r"{}\left({}\right)".format(self._latex_name,  scalar._latex_name) + latex_name = fr"{self._latex_name}\left({scalar._latex_name}\right)"Done in the last commit.
Great. I'll give it a positive review as soon as patchbot is green.
comment:6 Changed 6 months ago by
 Reviewers set to Michael Jung
 Status changed from needs_review to positive_review
Patchbot green. LGTM.
comment:7 Changed 6 months ago by
Thank you for the review!
comment:8 Changed 4 months ago by
 Branch changed from public/manifolds/vector_field_call_31633 to bc257815fcd42a6ba49eb0639ad1dc7e6a172f03
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
Simplify VectorField.__call__