Ease the display of tensor fields in a coordinate frame

[egourgoulhon]

Consider a manifold covered by two coordinate charts:

sage: M = Manifold(2, 'M')
sage: X.<x,y> = M.chart()
sage: Y.<u,v> = M.chart()
sage: X_to_Y = X.transition_map(Y, [x+y, x-y])
sage: Y_to_X = X_to_Y.inverse()

and define a vector field from its components in the manifold's default vector frame:

sage: M.default_frame()
Coordinate frame (M, (d/dx,d/dy))
sage: M.default_chart()
Chart (M, (x, y))
sage: v = M.vector_field(-y, x, name='v')
sage: v.display()
v = -y d/dx + x d/dy

Currently (Sage 8.7), if we would like to express v in terms of the coordinates (u,v), we have to write

sage: v.display(Y.frame(), Y)
v = v d/du - u d/dv

This is because the first argument of display is the vector frame with respect to which the expansion of v is performed and the second argument is the chart in which the components are expressed. If the latter is omitted, the default chart is assumed:

sage: v.display(Y.frame())
v = (x - y) d/du + (-x - y) d/dv

Now, it occurs quite often that one wants to express a tensor field entirely in terms of a given chart, i.e. with the components w.r.t. the coordinate frame associated to the chart and each component being expressed in terms of the coordinates of the chart. In Sage 8.7, if Y is not the default chart, this is possible only through v.display(Y.frame(), Y). This ticket introduces the possibility to pass only the chart to display, with the understanding that the vector frame with respect to which the expansion is to be performed is the coordinate frame associated with this chart. In other words, v.display(Y.frame(), Y) can now be shorten to v.display(Y):

sage: v.display(Y)
v = v d/du - u d/dv

CC: @tscrim

Component: geometry

Keywords: tensor field, coordinate frame

Author: Eric Gourgoulhon

Branch/Commit: 4060557

Reviewer: Travis Scrimshaw

Issue created by migration from https://trac.sagemath.org/ticket/27655

[egourgoulhon]

Branch: public/manifolds/display_from_chart

[egourgoulhon]

New commits:

5b2c1ab	Display of tensor fields with only a chart given as argument
9155312	Merge branch 'public/manifolds/display_from_chart' of git://trac.sagemath.org/sage into Sage 8.8.beta1.
d975e85	Update documentation with display of tensor fields with only a chart given as argument

[egourgoulhon]

Commit: d975e85

[tscrim]

comment:4

Is there a better way than ducktyping to check if frame is a coordinate chart? IIRC, there is a base class for all coordinate charts, why not check isinstance(frame, CoordinateChartBaseClass)? This is more robust as a test since if frame ever has a method frame, then the code would break (likely in a very strange way).

[sagetrac-git]

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

b97afe2

Check of chart with isinstance(basis, Chart) in FreeModuleTensor._preparse_display

[sagetrac-git]

Changed commit from d975e85 to b97afe2

[egourgoulhon]

comment:6

Replying to @tscrim:

Is there a better way than ducktyping to check if frame is a coordinate chart? IIRC, there is a base class for all coordinate charts, why not check isinstance(frame, CoordinateChartBaseClass)?

Done in the above commit.

This is more robust as a test since if frame ever has a method frame, then the code would break (likely in a very strange way).

Indeed.

[tscrim]

Reviewer: Travis Scrimshaw

[tscrim]

comment:7

Thank you. LGTM.

[egourgoulhon]

comment:8

Thanks for the review!

[vbraun]

comment:9

That causes quite a lot of breakage, see patchbot

[tscrim]

comment:10

Replying to @vbraun:

That causes quite a lot of breakage, see patchbot

Sorry for what is likely a dumb question, but are you sure it is this ticket? I just have absolutely no idea how those failures could be related to this ticket. I agree that all the patchbots for what seems to be just ticket have massive breakage, but it seems to come from the mpmath library, which is not touched by this ticket.

[vbraun]

comment:11

It went away after unmerging this ticket, so seems to be the case

[egourgoulhon]

comment:12

Replying to @vbraun:

It went away after unmerging this ticket, so seems to be the case

I confirm the errors reported by the patchbot: on my computer, after merging the ticket branch in Sage 8.8.beta2, I get

sage: complex(erf(3*I))
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-1-af6acac02c0b> in <module>()
----> 1 complex(erf(Integer(3)*I))

/home/eric/sage/8.8.develop/local/lib/python2.7/site-packages/sage/symbolic/expression.pyx in sage.symbolic.expression.Expression.__complex__ (build/cythonized/sage/symbolic/expression.cpp:11230)()
   1441             return self._eval_self(complex)
   1442         except TypeError:
-> 1443             raise TypeError("unable to simplify to complex approximation")
   1444 
   1445     def _sympy_(self):

TypeError: unable to simplify to complex approximation

The error raised in self._eval_self(complex) seems related to mpmath, as pointed in comment:10:

sage: erf(3*I)._eval_self(complex)
...
TypeError: Cannot convert mpz to sage.rings.integer.Integer

However I am completely puzzled: how could possibly the code in this branch alter mpmath?
We have

./sage -tp --long src/sage/manifolds/

returns "All tests passed!", but the other parts of Sage are impacted as the patchbot says, for instance

./sage -t --long --warn-long 48.3 src/sage/functions/exp_integral.py  

returns "93 doctests failed"...

[vbraun]

comment:13

It touches free modules so presumably something screws up mpmath as free module over integers...

[egourgoulhon]

comment:14

The culprit is the commit
sagemath/sagetrac-mirror@b97afe2
added in comment:5. It seems rather inoffensive, but if one displaces the import

from sage.manifolds.chart import Chart

from the top of the module src/sage/tensor/modules/free_module_tensor.py to the body of the method FreeModuleTensor._preparse_display, where Chart is actually required, then everything is OK. Could this be some clash with the name Chart and mpmath?

[sagetrac-git]

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

c529eb5	Merge branch 'public/manifolds/display_from_chart' of git://trac.sagemath.org/sage into Sage 8.8.beta2
5106067	Make the import of Chart local to FreeModuleTensor._preparse_display

[sagetrac-git]

Changed commit from b97afe2 to 5106067

[egourgoulhon]

comment:16

I am setting the ticket to "needs review" to draw the attention of the patchbot. The above commit, which simply makes the import of Chart local to FreeModuleTensor._preparse_display, seems to solve the issue. There remains to understand why the import at the module level caused such a mess with mpmath...

[sagetrac-git]

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

5536b94

Lazy import of FiniteRankFreeModule + import of Chart at the module level in free_module_tensor.py

[sagetrac-git]

Changed commit from 5106067 to 5536b94

[egourgoulhon]

comment:18

I think I found the real culprit: in src/sage/tensor/modules/all.py there was a direct import of FiniteRankFreeModule, which caused src/sage/tensor/modules/free_module_tensor to be loaded at Sage startup, and hence src/sage/manifolds/chart when the import of Chart was at the module level. I've changed the import of FiniteRankFreeModule to be a lazy one, so that src/sage/manifolds/chart (and all its imports) is no longer imported at Sage startup. Everything seems fixed now.

Since it is now harmless, I've restored the import of Chart at the module level in src/sage/tensor/modules/free_module_tensor.py, reverting what was done in the commit in comment:15. I am wondering however if this is a good thing... i.e. shall we keep the import of Chart at the level of the function FreeModuleTensor._preparse_display, in order not to mix (too much) the algebraic part (src/sage/tensor/modules) and the topological one (src/sage/manifolds)?

[tscrim]

comment:19

IMO, that is still somewhat of a hack fix, but far less of one that the other. I am wondering if what is going on is this is changing the overall import order of things into Sage, and somehow things are more sensitive to this ordering than we thought. That means dealing with Sage's import hell.

I am okay with the current state modulo an additional comment at the lazy import referencing this ticket and the general issue. Once done, positive review since it now passes on the patchbot (modulo one test which is unrelated).

[sagetrac-git]

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

093bd08

Check basis against a FreeModuleBasis in FreeModuleTensor._preparse_display

[sagetrac-git]

Changed commit from 5536b94 to 093bd08

[egourgoulhon]

comment:21

Replying to @egourgoulhon:

I am wondering however if this is a good thing... i.e. shall we keep the import of Chart at the level of the function FreeModuleTensor._preparse_display, in order not to mix (too much) the algebraic part (src/sage/tensor/modules) and the topological one (src/sage/manifolds)?

The more I think about it, the more I am convinced that we shall not import anything from src/sage/manifolds into src/sage/tensor/modules in order to keep the pure algebraic part completely separated from the manifold one. In the above commit, I propose to restore the ducktyping in the check of the basis nature of the first argument of FreeModuleTensor._preparse_display but in a much safer way than what was done previous to the commit in comment:5, namely by

elif not isinstance(basis, FreeModuleBasis):

instead of

elif isinstance(basis, Chart):

In this way, we do not have to import Chart from the manifold part, but only FreeModuleBasis, which pertains to the algebraic part. Moreover, it is much safer: if basis is a true module basis and (for some reason) has an attribute frame(), it will never mistakenly be replaced by basis.frame().

[egourgoulhon]

comment:22

Replying to @tscrim:

IMO, that is still somewhat of a hack fix, but far less of one that the other. I am wondering if what is going on is this is changing the overall import order of things into Sage, and somehow things are more sensitive to this ordering than we thought. That means dealing with Sage's import hell.

Indeed.

I am okay with the current state modulo an additional comment at the lazy import referencing this ticket and the general issue. Once done, positive review since it now passes on the patchbot (modulo one test which is unrelated).

In the current setting, the lazy import is no longer necessary (I have however kept it). Do you think the comment is still appropriate? Shall we revert to the direct import? What is the policy about lazy imports vs. direct imports. I've noticed that in the manifold part, all imports are lazy ones, except for the catalog, while here (src/sage/tensor/modules/all.py) it was a true import. I don't remember the reason about this... Was it a matter of performance?

[tscrim]

comment:23

Replying to @egourgoulhon:

Replying to @egourgoulhon:

I am wondering however if this is a good thing... i.e. shall we keep the import of Chart at the level of the function FreeModuleTensor._preparse_display, in order not to mix (too much) the algebraic part (src/sage/tensor/modules) and the topological one (src/sage/manifolds)?

The more I think about it, the more I am convinced that we shall not import anything from src/sage/manifolds into src/sage/tensor/modules in order to keep the pure algebraic part completely separated from the manifold one. In the above commit, I propose to restore the ducktyping in the check of the basis nature of the first argument of FreeModuleTensor._preparse_display but in a much safer way than what was done previous to the commit in comment:5, namely by

elif not isinstance(basis, FreeModuleBasis):

instead of

elif isinstance(basis, Chart):

In this way, we do not have to import Chart from the manifold part, but only FreeModuleBasis, which pertains to the algebraic part. Moreover, it is much safer: if basis is a true module basis and (for some reason) has an attribute frame(), it will never mistakenly be replaced by basis.frame().

I think that is a bad to false argument. The fact that this code means there is some interaction between the two. It is not that you want everything else to behave like a Chart, but you want specific behavior for a Chart. Thus, I think importing it is the correct thing to do. IMO, this falls under the "explicit is better than implicit" rule.

[tscrim]

comment:24

Replying to @egourgoulhon:

Replying to @tscrim:

I am okay with the current state modulo an additional comment at the lazy import referencing this ticket and the general issue. Once done, positive review since it now passes on the patchbot (modulo one test which is unrelated).

In the current setting, the lazy import is no longer necessary (I have however kept it). Do you think the comment is still appropriate? Shall we revert to the direct import? What is the policy about lazy imports vs. direct imports. I've noticed that in the manifold part, all imports are lazy ones, except for the catalog, while here (src/sage/tensor/modules/all.py) it was a true import. I don't remember the reason about this... Was it a matter of performance?

It has to do with startup time. A lazy import means that it will not get resolved right away, which means things that are not needed at startup are not loaded. Since the manifolds has a lot of imports internally, we want to lazily import the block rather than do it at startup. While the import is usually not something we typically notice as humans, the startup time issue has a lot of these and is death by a 1000 needles.

[sagetrac-git]

Changed commit from 093bd08 to 4060557

[sagetrac-git]

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

4060557

Check basis against a Chart in FreeModuleTensor._preparse_display

[egourgoulhon]

comment:26

Replying to @tscrim:

I think that is a bad to false argument. The fact that this code means there is some interaction between the two. It is not that you want everything else to behave like a Chart, but you want specific behavior for a Chart.

True. Having to deal with a Chart object at this level is a sign of bad design. A clean solution would be to redefine _preparse_display in the manifold part, i.e. as TensorFieldParal._preparse_display. But this would require to rearrange the inheritance structure of tensor fields. For instance, at the moment we have

class VectorFieldParal(FiniteRankFreeModuleElement, MultivectorFieldParal, VectorField)

so that for a vector field v, v._preparse_display will invoke FreeModuleTensor._preparse_display, not TensorFieldParal._preparse_display. We need the MRO to be instead

class VectorFieldParal(MultivectorFieldParal, FiniteRankFreeModuleElement, VectorField)

Clearly, such a reorganization is beyond the scope of the current ticket and deserves its own ticket. So, for the time being, I've kept the import of Chart in free_module_tensor.py and have added a TODO comment to it. I've also added a comment about the lazy import in all.py, as suggested in comment:19.

Thus, I think importing it is the correct thing to do. IMO, this falls under the "explicit is better than implicit" rule.

OK. As said above, we have now the explicit import of Chart, along with a TODO comment for a future reorganization.

[egourgoulhon]

comment:27

Replying to @tscrim:

Replying to @egourgoulhon:

In the current setting, the lazy import is no longer necessary (I have however kept it). Do you think the comment is still appropriate? Shall we revert to the direct import? What is the policy about lazy imports vs. direct imports. I've noticed that in the manifold part, all imports are lazy ones, except for the catalog, while here (src/sage/tensor/modules/all.py) it was a true import. I don't remember the reason about this... Was it a matter of performance?

It has to do with startup time. A lazy import means that it will not get resolved right away, which means things that are not needed at startup are not loaded. Since the manifolds has a lot of imports internally, we want to lazily import the block rather than do it at startup. While the import is usually not something we typically notice as humans, the startup time issue has a lot of these and is death by a 1000 needles.

Thanks for your reply. It's clearly a good thing to set a lazy import for FiniteRankFreeModule to decrease Sage startup time. Shouldn't we do the same thing for sage.manifolds.catalog in src/sage/manifolds/all.py? (I plan to work on the manifold catalog soon anyway, so that I could perform this change at this occasion).

[tscrim]

comment:28

Replying to @egourgoulhon:

Replying to @tscrim:

Replying to @egourgoulhon:

In the current setting, the lazy import is no longer necessary (I have however kept it). Do you think the comment is still appropriate? Shall we revert to the direct import? What is the policy about lazy imports vs. direct imports. I've noticed that in the manifold part, all imports are lazy ones, except for the catalog, while here (src/sage/tensor/modules/all.py) it was a true import. I don't remember the reason about this... Was it a matter of performance?

It has to do with startup time. A lazy import means that it will not get resolved right away, which means things that are not needed at startup are not loaded. Since the manifolds has a lot of imports internally, we want to lazily import the block rather than do it at startup. While the import is usually not something we typically notice as humans, the startup time issue has a lot of these and is death by a 1000 needles.

Thanks for your reply. It's clearly a good thing to set a lazy import for FiniteRankFreeModule to decrease Sage startup time. Shouldn't we do the same thing for sage.manifolds.catalog in src/sage/manifolds/all.py? (I plan to work on the manifold catalog soon anyway, so that I could perform this change at this occasion).

This is not something of any real benefit as just that file is loaded (all of the other imports are done within the functions). The bigger things that need to be lazily imported are a lot of things in combinat (IIRC, Nicolas did a big lazy import and startup time dropped by a third) and some of the other highly specialized features that are not performance critical.

TL;DR. I wouldn't do that.

[tscrim]

comment:29

Replying to @egourgoulhon:

Replying to @tscrim:

I think that is a bad to false argument. The fact that this code means there is some interaction between the two. It is not that you want everything else to behave like a Chart, but you want specific behavior for a Chart.

True. Having to deal with a Chart object at this level is a sign of bad design. A clean solution would be to redefine _preparse_display in the manifold part, i.e. as TensorFieldParal._preparse_display. But this would require to rearrange the inheritance structure of tensor fields. For instance, at the moment we have

class VectorFieldParal(FiniteRankFreeModuleElement, MultivectorFieldParal, VectorField)

so that for a vector field v, v._preparse_display will invoke FreeModuleTensor._preparse_display, not TensorFieldParal._preparse_display. We need the MRO to be instead

class VectorFieldParal(MultivectorFieldParal, FiniteRankFreeModuleElement, VectorField)

Clearly, such a reorganization is beyond the scope of the current ticket and deserves its own ticket. So, for the time being, I've kept the import of Chart in free_module_tensor.py and have added a TODO comment to it. I've also added a comment about the lazy import in all.py, as suggested in comment:19.

I will leave the higher level decisions to you on that. Just let me know what you want me to review. ;)

Thus, I think importing it is the correct thing to do. IMO, this falls under the "explicit is better than implicit" rule.

OK. As said above, we have now the explicit import of Chart, along with a TODO comment for a future reorganization.

Thank you. LGTM.

[egourgoulhon]

comment:30

Thanks for the review and the discussion!

[vbraun]

Changed branch from public/manifolds/display_from_chart to 4060557