Opened 7 months ago

Closed 7 months ago

#27655 closed enhancement (fixed)

Ease the display of tensor fields in a coordinate frame

Reported by: egourgoulhon Owned by:
Priority: major Milestone: sage-8.8
Component: geometry Keywords: tensor field, coordinate frame, manifolds
Cc: tscrim Merged in:
Authors: Eric Gourgoulhon Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 4060557 (Commits) Commit: 4060557d205263fd5c8a2ce2e1d0e0e96e5baa94
Dependencies: Stopgaps:

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

Change History (31)

comment:1 Changed 7 months ago by egourgoulhon

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

New commits:

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

comment:2 Changed 7 months 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 7 months ago by egourgoulhon

  • Cc tscrim added
  • Status changed from new to needs_review

comment:4 follow-up: Changed 7 months 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 7 months ago by git

  • Commit changed from d975e8599f22a73f6ba371e8491592e06c9d0c53 to b97afe2304d8dcfa4c41a2c6d5b68685373839c6

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

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

comment:6 in reply to: ↑ 4 Changed 7 months ago by egourgoulhon

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.

comment:7 Changed 7 months ago by tscrim

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

Thank you. LGTM.

comment:8 Changed 7 months ago by egourgoulhon

Thanks for the review!

comment:9 follow-up: Changed 7 months 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 7 months ago by tscrim

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.

comment:11 follow-up: Changed 7 months ago by vbraun

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

comment:12 in reply to: ↑ 11 Changed 7 months ago by egourgoulhon

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

comment:13 Changed 7 months ago by vbraun

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

comment:14 Changed 7 months 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 7 months ago by git

  • Commit changed from b97afe2304d8dcfa4c41a2c6d5b68685373839c6 to 5106067e6b4c30f6167ef3f2531232b94f99184d

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

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

comment:16 Changed 7 months 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 7 months ago by git

  • Commit changed from 5106067e6b4c30f6167ef3f2531232b94f99184d to 5536b9431def2dce5a78f90a9b2a33c495192060

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

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

comment:18 follow-up: Changed 7 months 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: Changed 7 months 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 7 months ago by git

  • Commit changed from 5536b9431def2dce5a78f90a9b2a33c495192060 to 093bd082a9d1129444e7fa5085796cac39fc881d

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

093bd08Check basis against a FreeModuleBasis in FreeModuleTensor._preparse_display

comment:21 in reply to: ↑ 18 ; follow-up: Changed 7 months ago by 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().

comment:22 in reply to: ↑ 19 ; follow-up: Changed 7 months ago by egourgoulhon

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?

comment:23 in reply to: ↑ 21 ; follow-up: Changed 7 months ago by tscrim

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.

comment:24 in reply to: ↑ 22 ; follow-up: Changed 7 months ago by tscrim

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.

comment:25 Changed 7 months ago by git

  • Commit changed from 093bd082a9d1129444e7fa5085796cac39fc881d to 4060557d205263fd5c8a2ce2e1d0e0e96e5baa94

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

4060557Check basis against a Chart in FreeModuleTensor._preparse_display

comment:26 in reply to: ↑ 23 ; follow-up: Changed 7 months ago by 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.

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: Changed 7 months ago by 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).

comment:28 in reply to: ↑ 27 Changed 7 months ago by tscrim

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.

comment:29 in reply to: ↑ 26 Changed 7 months ago by tscrim

  • Status changed from needs_review to positive_review

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.

comment:30 Changed 7 months ago by egourgoulhon

Thanks for the review and the discussion!

comment:31 Changed 7 months 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.