Opened 23 months ago
Closed 23 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, GitHub, GitLab) | 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 23 months ago by
- Branch set to public/manifolds/display_from_chart
- Commit set to d975e8599f22a73f6ba371e8491592e06c9d0c53
comment:2 Changed 23 months ago by
- 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 23 months ago by
- Cc tscrim added
- Status changed from new to needs_review
comment:4 follow-up: ↓ 6 Changed 23 months ago by
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 23 months ago by
- 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 23 months ago by
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 checkisinstance(frame, CoordinateChartBaseClass)
?
Done in the above commit.
This is more robust as a test since if
frame
ever has a methodframe
, then the code would break (likely in a very strange way).
Indeed.
comment:7 Changed 23 months ago by
- Reviewers set to Travis Scrimshaw
- Status changed from needs_review to positive_review
Thank you. LGTM.
comment:8 Changed 23 months ago by
Thanks for the review!
comment:9 follow-up: ↓ 10 Changed 23 months ago by
- Status changed from positive_review to needs_work
That causes quite a lot of breakage, see patchbot
comment:10 in reply to: ↑ 9 Changed 23 months ago by
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: ↓ 12 Changed 23 months ago by
It went away after unmerging this ticket, so seems to be the case
comment:12 in reply to: ↑ 11 Changed 23 months ago by
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 23 months ago by
It touches free modules so presumably something screws up mpmath as free module over integers...
comment:14 Changed 23 months ago by
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 23 months ago by
- Commit changed from b97afe2304d8dcfa4c41a2c6d5b68685373839c6 to 5106067e6b4c30f6167ef3f2531232b94f99184d
comment:16 Changed 23 months ago by
- 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 23 months ago by
- 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 23 months ago by
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 23 months ago by
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 23 months ago by
- 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 23 months ago by
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 functionFreeModuleTensor._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: ↓ 24 Changed 23 months ago by
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: ↓ 26 Changed 23 months ago by
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 functionFreeModuleTensor._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
intosrc/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 ofFreeModuleTensor._preparse_display
but in a much safer way than what was done previous to the commit in comment:5, namely byelif 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 onlyFreeModuleBasis
, which pertains to the algebraic part. Moreover, it is much safer: ifbasis
is a true module basis and (for some reason) has an attributeframe()
, it will never mistakenly be replaced bybasis.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 23 months ago by
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 23 months ago by
- 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 23 months ago by
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 aChart
.
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 23 months ago by
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 23 months ago by
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 forsage.manifolds.catalog
insrc/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 23 months ago by
- 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 aChart
.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. asTensorFieldParal._preparse_display
. But this would require to rearrange the inheritance structure of tensor fields. For instance, at the moment we haveclass VectorFieldParal(FiniteRankFreeModuleElement, MultivectorFieldParal, VectorField)so that for a vector field
v
,v._preparse_display
will invokeFreeModuleTensor._preparse_display
, notTensorFieldParal._preparse_display
. We need the MRO to be insteadclass 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
infree_module_tensor.py
and have added a TODO comment to it. I've also added a comment about the lazy import inall.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 23 months ago by
Thanks for the review and the discussion!
comment:31 Changed 23 months ago by
- Branch changed from public/manifolds/display_from_chart to 4060557d205263fd5c8a2ce2e1d0e0e96e5baa94
- Resolution set to fixed
- Status changed from positive_review to closed
New commits:
Display of tensor fields with only a chart given as argument
Merge branch 'public/manifolds/display_from_chart' of git://trac.sagemath.org/sage into Sage 8.8.beta1.
Update documentation with display of tensor fields with only a chart given as argument