Opened 2 years ago

Closed 21 months ago

#25866 closed enhancement (fixed)

Tensor series expansion

Reported by: gh-FlorentinJ Owned by:
Priority: major Milestone: sage-8.8
Component: geometry Keywords: manifold, expansion, series, tensor
Cc: egourgoulhon Merged in:
Authors: Florentin Jaffredo Reviewers: Eric Gourgoulhon, Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: beaa20e (Commits) Commit: beaa20e9f75805a8a9a4476195b8aeaee34876aa
Dependencies: Stopgaps:

Description

This ticket modifies the method _simplify of chart functions on manifolds to permit series expansion of tensor field, scalar field or connection.

Most of the usual operations are already supported (except Lie derivative).

Because the expansion is performed at the lowest level, it provides a major improvement in performances for computing general perturbations (compared with simply expanding after the computation).

A notebook example can be found here.

This is part of the ​SageManifolds project; see the metaticket #18528 for an overview.

Change History (29)

comment:1 Changed 2 years ago by gh-FlorentinJ

  • Status changed from new to needs_review

comment:2 Changed 2 years ago by egourgoulhon

  • Status changed from needs_review to needs_work

Thank you very much for having added this new functionality! The code looks basically good to me. There are some issues with the user interface and documentation though:

  • Method ChartFunction.__init__(): the new arguments symbol and order must added in the INPUT section of the documentation of ChartFunction; besides the name symbol is not explicit enough; it should be replaced by something like expansion_symbol
  • Method TensorField.set_calc_order(): no explicit call to that method appears in its doctests!
  • Method TensorFieldParal.series(): it would help the reader if you add the raw output of this method in the doctests, i.e. add
     sage: g.series(e, 3)
    [(Field of symmetric bilinear forms on the 4-dimensional Lorentzian manifold M,
      0),
     (Field of symmetric bilinear forms on the 4-dimensional Lorentzian manifold M,
      1),
     (Field of symmetric bilinear forms on the 4-dimensional Lorentzian manifold M,
      2)]
    
    before displaying the individual elements of the returned list
  • In the doctests of the same method and as well as in truncate() and set_calc_order(), note that you can use
    sage: g.set(g+e*h1+e**2*h2)
    
    as a shortcut for
    sage: g.set_comp()[:] = (g+e*h1+e**2*h2)[:]
    
  • Method PseudoRiemannianMetric.inverse(): the name symbol for the argument that triggers the expansion is not explicit enough: it should be changed to something like expand_with_respect_to.
  • Method ScalarField.set_calc_order(): the docstring (probably cut-and-pasted from the tensor field one) must be adapted to the scalar field case; in particular components has no meaning in the current context; besides Tell the ... does not look like a proper phrase for a reference manual...

comment:3 Changed 2 years ago by egourgoulhon

  • Reviewers set to Eric Gourgoulhon

comment:4 Changed 2 years ago by git

  • Commit changed from eea81a546948575d7b46870a6bb33780049fce59 to 154314adada09e3a38a38a73476d7101e8653cc7

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

b20a041Merge branch 'public/manifolds/series' of git://trac.sagemath.org/sage into public/manifolds/series
154314aAddressing Eric's comments.

comment:5 follow-up: Changed 2 years ago by tscrim

This should take care of comment:2 Eric. However, the doctest for set_calc_order in tensorfield.py and tensorfield_paral.py are the same. Florentin, Eric, could one of you get a modified doctest that calls that method specifically?

comment:6 Changed 2 years ago by git

  • Commit changed from 154314adada09e3a38a38a73476d7101e8653cc7 to 865c5481e0f1ede6aa3ffa85395f56c1ca2b4a86

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

ab5d292Merge branch 'public/manifolds/series' of git://trac.sagemath.org/sage into series_expansion
865c548Correct some doctests in series expansions of tensor fields

comment:7 in reply to: ↑ 5 ; follow-up: Changed 2 years ago by egourgoulhon

Replying to tscrim:

This should take care of comment:2 Eric. However, the doctest for set_calc_order in tensorfield.py and tensorfield_paral.py are the same. Florentin, Eric, could one of you get a modified doctest that calls that method specifically?

Thanks for your changes and apologies for the delay in replying. The above commit fixes some doctests and provides a specific doctest for set_calc_order in tensorfield.py. There are still some documentation issues, that I shall address in a next commit...

comment:8 Changed 2 years ago by git

  • Commit changed from 865c5481e0f1ede6aa3ffa85395f56c1ca2b4a86 to 75bc8cb90d7029e80ac21addb1a80489428087ec

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

75bc8cbImprovements in the documentation of series expansions of tensor fields

comment:9 in reply to: ↑ 7 Changed 2 years ago by egourgoulhon

Replying to egourgoulhon:

There are still some documentation issues, that I shall address in a next commit...

Done in the above commit.

A question remains: the argument order of the various methods set_calc_order, as well as the attribute _order, is the order of the big O. As a result, to get an expansion to first order with respect to the parameter epsilon, we have to do g.set_calc_order(epsilon, 2). This seems very counter-intuitive and error-prone to me: I would have expected g.set_calc_order(epsilon, 1) instead. I understand that the current setting follows the convention of the method series of symbolic expressions, while I would prefer that it follows the convention of taylor:

sage: exp(x).series(x, 2)
1 + 1*x + Order(x^2)
sage: exp(x).taylor(x, 0, 1)
x + 1

(Btw, the above looks like a kind of inconsistency in Sage)

What do you think?

comment:10 Changed 2 years ago by egourgoulhon

  • Status changed from needs_work to needs_info

comment:11 follow-up: Changed 2 years ago by tscrim

I think Sage is not inconsistent as the input to taylor is degree, not order. However, I do agree that there is some ambiguity on which order it is referring too (i.e., first order approximation vs big O order). You could make the case either way. I don't have an opinion either way, but I think both ways are likely to lead to errors. It may be better to call it set_calc_degree to sidestep this issue. Although if you prefer to keep degree, I think either way is fine as long as it is documented with perhaps a .. WARNING:: block or two.

comment:12 in reply to: ↑ 11 Changed 2 years ago by egourgoulhon

Replying to tscrim:

I think Sage is not inconsistent as the input to taylor is degree, not order. However, I do agree that there is some ambiguity on which order it is referring too (i.e., first order approximation vs big O order). You could make the case either way. I don't have an opinion either way, but I think both ways are likely to lead to errors.

Well, at least in physics, I would say that the order of an expansion always stands for the degree of the polynomial that approximates the function, not for the degree of the big O. I am pretty sure that this is what the user has in mind when asking for a n-th order computation. Since in the currrent context the big O is not even displayed in the result, I would really favor this interpretation of order.

It may be better to call it set_calc_degree to sidestep this issue. Although if you prefer to keep degree, I think either way is fine as long as it is documented with perhaps a .. WARNING:: block or two.

In the current context, calculation degree sounds awkward to me, contrary to calculation order. Speaking about set_calc_order, are we sure that this is a good name in the first place? This method is indeed the user interface to indicate that one wants to perform an approximate calculation. Would set_series_expansion or use_series_expansion or something else be a better name?

comment:13 Changed 2 years ago by tscrim

I don't really are which one we take, but if we use order, it just has to be well documented IMO. I am not sure about series_expansion as that doesn't feel like the correct thing; it is something I would never guess. I think set_calc_order is probably the best we can do; so just documenting what we mean should be enough.

comment:14 Changed 21 months ago by git

  • Commit changed from 75bc8cb90d7029e80ac21addb1a80489428087ec to b8100119f3b9a5f7f7f8b8da6e24e201a7c9241d

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

131ee7dMerge branch 'public/manifolds/series' of git://trac.sagemath.org/sage into Sage 8.7.beta7.
10356cfSet order to be the degree of the approximating polynomial in tensor series expansions; improve documentation
b810011Rename method series() to series_expansion() in tensor fields and make it return a list of tensor fields

comment:15 Changed 21 months ago by egourgoulhon

Sorry for the long delay in coming back to this! In the above commits, I have

  • set order to be the degree of the polynomial in front of the big O, adding some warnings about this (cf. the discussion in comment:12 and comment:13)
  • simplified the output of the method TensorFieldParal.series() (there was not need to return a list of pairs with the second element of each pair being merely the index in the list)
  • add the arguments expansion_symbol and order to the method Chart.function() for consistency with the constructor of a chart function
  • improved the documentation, in particular regarding the meaning of order

Besides I've run some benchmarks to check that the new functionalities added by this ticket, which are essentially implemented in ChartFunction._simplify(), do not degrade significantly the tensor calculus performance when no series expansion is required.

comment:16 Changed 21 months ago by egourgoulhon

  • Status changed from needs_info to needs_review

comment:17 Changed 21 months ago by egourgoulhon

  • Milestone changed from sage-8.4 to sage-8.7

comment:18 Changed 21 months ago by tscrim

I just have some nitpicks that once changed, you can set a positive review on my behalf.

I think this will look at little better in the compiled doc:

-`f_0`, `f_1`, ..., `f_n`
+`f_0, f_1, \ldots, f_n`

and similarly for the T's. (Although I am not sure I have actually ever checked this...)

In all of the .. MATH:: (that are followed by a where), you should add a comma to the end of the formulas for grammatical correctness.

I always think of big O with the O appearing as a math symbol, so I would say it should be formatted as big `O` in the doc.

comment:19 Changed 21 months ago by tscrim

  • Reviewers changed from Eric Gourgoulhon to Eric Gourgoulhon, Travis Scrimshaw

comment:20 Changed 21 months ago by git

  • Commit changed from b8100119f3b9a5f7f7f8b8da6e24e201a7c9241d to 87a256bb56c8e09b6b09550ea2fe1c194b00aa5b

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

87a256bImprove documentation in series expansion of tensor fields

comment:21 follow-up: Changed 21 months ago by tscrim

  • Status changed from needs_review to positive_review

Thanks. LGTM.

comment:22 in reply to: ↑ 21 Changed 21 months ago by egourgoulhon

Replying to tscrim: Thanks for comment:18 and the review!

comment:23 follow-up: Changed 21 months ago by egourgoulhon

  • Status changed from positive_review to needs_work

With the python3 version of Sage 8.7.rc0, building the documentation via

./sage -docbuild reference/manifolds html

generates the following warnings:

[manifolds] <unknown>:2105: DeprecationWarning: invalid escape sequence \e
[manifolds] <unknown>:2243: DeprecationWarning: invalid escape sequence \e

(there was no such warning with the python2 version).

I suspect the issue could be triggered by the \epsilon's introduced in the documentation. The documentation output looks fine though... Moreover the error message is not very explicit...

comment:24 in reply to: ↑ 23 Changed 21 months ago by egourgoulhon

Replying to egourgoulhon:

I suspect the issue could be triggered by the \epsilon's introduced in the documentation. The documentation output looks fine though... Moreover the error message is not very explicit...

The offending file is src/sage/manifolds/differentiable/tensorfield_paral.py and the numbers 2105 and 2243 in the error message correspond to the ending lines of the docstring of the methods series_expansion and set_calc_order respectively. I looked through the file and did not find any obvious error... Again the html output is fine (all math formulas involving \epsilon are correctly rendered).

comment:25 Changed 21 months ago by git

  • Commit changed from 87a256bb56c8e09b6b09550ea2fe1c194b00aa5b to beaa20e9f75805a8a9a4476195b8aeaee34876aa

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

beaa20eCorrect docstrings opening in methods series_expansion and set_calc_order of tensorfield_paral.py

comment:26 Changed 21 months ago by egourgoulhon

  • Status changed from needs_work to needs_review

OK, I found the reason: the docstrings of the two involved methods were starting with """ instead of r""".

comment:27 Changed 21 months ago by tscrim

  • Status changed from needs_review to positive_review

Python3 is very strict about escape sequences, either it is one (which gives a rendering error in Python2) or it raises an error (whereas Python2 silently ignores it).

comment:28 Changed 21 months ago by embray

  • Milestone changed from sage-8.7 to sage-8.8

Ticket retargeted after milestone closed (if you don't believe this ticket is appropriate for the Sage 8.8 release please retarget manually)

comment:29 Changed 21 months ago by vbraun

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