Opened 3 years ago
Closed 2 years ago
#25866 closed enhancement (fixed)
Tensor series expansion
Reported by:  ghFlorentinJ  Owned by:  

Priority:  major  Milestone:  sage8.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, GitHub, GitLab)  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 3 years ago by
 Status changed from new to needs_review
comment:2 Changed 3 years ago by
 Status changed from needs_review to needs_work
comment:3 Changed 3 years ago by
 Reviewers set to Eric Gourgoulhon
comment:4 Changed 3 years ago by
 Commit changed from eea81a546948575d7b46870a6bb33780049fce59 to 154314adada09e3a38a38a73476d7101e8653cc7
comment:5 followup: ↓ 7 Changed 3 years ago by
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
 Commit changed from 154314adada09e3a38a38a73476d7101e8653cc7 to 865c5481e0f1ede6aa3ffa85395f56c1ca2b4a86
comment:7 in reply to: ↑ 5 ; followup: ↓ 9 Changed 2 years ago by
Replying to tscrim:
This should take care of comment:2 Eric. However, the doctest for
set_calc_order
intensorfield.py
andtensorfield_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
 Commit changed from 865c5481e0f1ede6aa3ffa85395f56c1ca2b4a86 to 75bc8cb90d7029e80ac21addb1a80489428087ec
Branch pushed to git repo; I updated commit sha1. New commits:
75bc8cb  Improvements in the documentation of series expansions of tensor fields

comment:9 in reply to: ↑ 7 Changed 2 years ago by
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 counterintuitive and errorprone 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
 Status changed from needs_work to needs_info
comment:11 followup: ↓ 12 Changed 2 years ago by
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
Replying to tscrim:
I think Sage is not inconsistent as the input to
taylor
isdegree
, notorder
. 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 nth 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
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 2 years ago by
 Commit changed from 75bc8cb90d7029e80ac21addb1a80489428087ec to b8100119f3b9a5f7f7f8b8da6e24e201a7c9241d
Branch pushed to git repo; I updated commit sha1. New commits:
131ee7d  Merge branch 'public/manifolds/series' of git://trac.sagemath.org/sage into Sage 8.7.beta7.

10356cf  Set order to be the degree of the approximating polynomial in tensor series expansions; improve documentation

b810011  Rename method series() to series_expansion() in tensor fields and make it return a list of tensor fields

comment:15 Changed 2 years ago by
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
andorder
to the methodChart.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 2 years ago by
 Status changed from needs_info to needs_review
comment:17 Changed 2 years ago by
 Milestone changed from sage8.4 to sage8.7
comment:18 Changed 2 years ago by
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 2 years ago by
 Reviewers changed from Eric Gourgoulhon to Eric Gourgoulhon, Travis Scrimshaw
comment:20 Changed 2 years ago by
 Commit changed from b8100119f3b9a5f7f7f8b8da6e24e201a7c9241d to 87a256bb56c8e09b6b09550ea2fe1c194b00aa5b
Branch pushed to git repo; I updated commit sha1. New commits:
87a256b  Improve documentation in series expansion of tensor fields

comment:21 followup: ↓ 22 Changed 2 years ago by
 Status changed from needs_review to positive_review
Thanks. LGTM.
comment:22 in reply to: ↑ 21 Changed 2 years ago by
Replying to tscrim: Thanks for comment:18 and the review!
comment:23 followup: ↓ 24 Changed 2 years ago by
 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 2 years ago by
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 2 years ago by
 Commit changed from 87a256bb56c8e09b6b09550ea2fe1c194b00aa5b to beaa20e9f75805a8a9a4476195b8aeaee34876aa
Branch pushed to git repo; I updated commit sha1. New commits:
beaa20e  Correct docstrings opening in methods series_expansion and set_calc_order of tensorfield_paral.py

comment:26 Changed 2 years ago by
 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 2 years ago by
 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 2 years ago by
 Milestone changed from sage8.7 to sage8.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 2 years ago by
 Branch changed from public/manifolds/series to beaa20e9f75805a8a9a4476195b8aeaee34876aa
 Resolution set to fixed
 Status changed from positive_review to closed
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:
ChartFunction.__init__()
: the new argumentssymbol
andorder
must added in theINPUT
section of the documentation ofChartFunction
; besides the namesymbol
is not explicit enough; it should be replaced by something likeexpansion_symbol
TensorField.set_calc_order()
: no explicit call to that method appears in its doctests!TensorFieldParal.series()
: it would help the reader if you add the raw output of this method in the doctests, i.e. add before displaying the individual elements of the returned listtruncate()
andset_calc_order()
, note that you can use as a shortcut forPseudoRiemannianMetric.inverse()
: the namesymbol
for the argument that triggers the expansion is not explicit enough: it should be changed to something likeexpand_with_respect_to
.ScalarField.set_calc_order()
: the docstring (probably cutandpasted 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...