Opened 14 months ago
Closed 12 months ago
#31633 closed enhancement (fixed)
Simplify VectorField.__call__
Reported by: | egourgoulhon | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-9.4 |
Component: | manifolds | Keywords: | vector field, scalar field |
Cc: | tscrim, gh-mjungmath | 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 1-form. 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 14 months ago by
- Branch set to public/manifolds/vector_field_call_31633
- Cc tscrim gh-mjungmath added
- Commit set to c921e9738ac5d828eec7f8a0b0cbd60eba81a9f0
- Status changed from new to needs_review
comment:2 follow-up: ↓ 4 Changed 14 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 f-strings:
- 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 14 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 f-string in VectorField.__call__
|
comment:4 in reply to: ↑ 2 ; follow-up: ↓ 5 Changed 14 months ago by
Replying to gh-mjungmath:
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 vi df/dxi in all possible coordinate frames d/dxi, thereby triggering the evaluation of v in these frames. In the new code, the computation is performed as (df)i vi 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 f-strings:
- 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 14 months ago by
Replying to egourgoulhon:
It's because in the previous
VectorFieldParal.__call__
code, the computation was performed as vi df/dxi in all possible coordinate frames d/dxi, thereby triggering the evaluation of v in these frames. In the new code, the computation is performed as (df)i vi 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 f-strings:
- 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 14 months ago by
- Reviewers set to Michael Jung
- Status changed from needs_review to positive_review
Patchbot green. LGTM.
comment:7 Changed 14 months ago by
Thank you for the review!
comment:8 Changed 12 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__