Opened 3 years ago

Closed 3 years ago

Ease the display of tensor fields in a coordinate frame

Reported by: Owned by: egourgoulhon major sage-8.8 geometry tensor field, coordinate frame, manifolds tscrim Eric Gourgoulhon Travis Scrimshaw N/A 4060557 4060557d205263fd5c8a2ce2e1d0e0e96e5baa94

Description

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
```

comment:1 Changed 3 years ago by egourgoulhon

• Branch set to public/manifolds/display_from_chart
• Commit set to d975e8599f22a73f6ba371e8491592e06c9d0c53

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`

comment:2 Changed 3 years ago by egourgoulhon

• Summary changed from Improve the display of tensor fields in a coordinate frame to Ease the display of tensor fields in a coordinate frame

comment:3 Changed 3 years ago by egourgoulhon

• Status changed from new to needs_review

comment:4 follow-up: ↓ 6 Changed 3 years ago by 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)`? 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).

comment:5 Changed 3 years ago by git

• Commit changed from d975e8599f22a73f6ba371e8491592e06c9d0c53 to b97afe2304d8dcfa4c41a2c6d5b68685373839c6

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

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

comment:6 in reply to: ↑ 4 Changed 3 years ago by egourgoulhon

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.

comment:7 Changed 3 years ago by tscrim

• Reviewers set to Travis Scrimshaw
• Status changed from needs_review to positive_review

Thank you. LGTM.

comment:8 Changed 3 years ago by egourgoulhon

Thanks for the review!

comment:9 follow-up: ↓ 10 Changed 3 years ago by vbraun

• Status changed from positive_review to needs_work

That causes quite a lot of breakage, see patchbot

comment:10 in reply to: ↑ 9 Changed 3 years ago by tscrim

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.

comment:11 follow-up: ↓ 12 Changed 3 years ago by vbraun

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

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

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

comment:13 Changed 3 years ago by vbraun

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

comment:14 Changed 3 years ago by egourgoulhon

The culprit is the commit https://git.sagemath.org/sage.git/commit/?id=b97afe2304d8dcfa4c41a2c6d5b68685373839c6 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?

comment:15 Changed 3 years ago by git

• Commit changed from b97afe2304d8dcfa4c41a2c6d5b68685373839c6 to 5106067e6b4c30f6167ef3f2531232b94f99184d

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`

comment:16 Changed 3 years ago by egourgoulhon

• Status changed from needs_work to needs_review

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

comment:17 Changed 3 years ago by git

• Commit changed from 5106067e6b4c30f6167ef3f2531232b94f99184d to 5536b9431def2dce5a78f90a9b2a33c495192060

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`

comment:18 follow-up: ↓ 21 Changed 3 years ago by egourgoulhon

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`)?

comment:19 follow-up: ↓ 22 Changed 3 years ago by 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.

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

comment:20 Changed 3 years ago by git

• Commit changed from 5536b9431def2dce5a78f90a9b2a33c495192060 to 093bd082a9d1129444e7fa5085796cac39fc881d

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

 ​093bd08 `Check basis against a FreeModuleBasis in FreeModuleTensor._preparse_display`

comment:21 in reply to: ↑ 18 ; follow-up: ↓ 23 Changed 3 years ago by 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):
```

```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()`.

comment:22 in reply to: ↑ 19 ; follow-up: ↓ 24 Changed 3 years ago by egourgoulhon

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?

comment:23 in reply to: ↑ 21 ; follow-up: ↓ 26 Changed 3 years ago by tscrim

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):
```

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

comment:24 in reply to: ↑ 22 ; follow-up: ↓ 27 Changed 3 years ago by 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.

comment:25 Changed 3 years ago by git

• Commit changed from 093bd082a9d1129444e7fa5085796cac39fc881d to 4060557d205263fd5c8a2ce2e1d0e0e96e5baa94

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

 ​4060557 `Check basis against a Chart in FreeModuleTensor._preparse_display`

comment:26 in reply to: ↑ 23 ; follow-up: ↓ 29 Changed 3 years ago by egourgoulhon

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.

comment:27 in reply to: ↑ 24 ; follow-up: ↓ 28 Changed 3 years ago by 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).

comment:28 in reply to: ↑ 27 Changed 3 years ago by tscrim

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.

comment:29 in reply to: ↑ 26 Changed 3 years ago by tscrim

• Status changed from needs_review to positive_review

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.

comment:30 Changed 3 years ago by egourgoulhon

Thanks for the review and the discussion!

comment:31 Changed 3 years ago by vbraun

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