Opened 6 months ago

Closed 4 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:

Status badges

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 6 months ago by egourgoulhon

  • 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

New commits:

c921e97Simplify VectorField.__call__

comment:2 follow-up: Changed 6 months ago by gh-mjungmath

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 6 months ago by git

  • Commit changed from c921e9738ac5d828eec7f8a0b0cbd60eba81a9f0 to bc257815fcd42a6ba49eb0639ad1dc7e6a172f03

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

bc25781Combine raw string and f-string in VectorField.__call__

comment:4 in reply to: ↑ 2 ; follow-up: Changed 6 months ago by egourgoulhon

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 6 months ago by gh-mjungmath

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

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 6 months ago by gh-mjungmath

  • Reviewers set to Michael Jung
  • Status changed from needs_review to positive_review

Patchbot green. LGTM.

comment:7 Changed 6 months ago by egourgoulhon

Thank you for the review!

comment:8 Changed 4 months ago by vbraun

  • Branch changed from public/manifolds/vector_field_call_31633 to bc257815fcd42a6ba49eb0639ad1dc7e6a172f03
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.