-
-
Notifications
You must be signed in to change notification settings - Fork 488
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Speed up constructing high-dimensional Euclidean spaces #30061
Comments
comment:1
Most of the time spent seems to come from SR (Maxima). Is it possible to speed things up for this easy special case, avoiding symbolic computation completely? |
comment:2
Most of the CPU time is spent in step 2, actually in creating the symbolic variables (elements of
and we have:
Actually, it turns out that what takes most of CPU time is demanding that the symbolic variables are real. Indeed, if we skip
I don't know why NB: in the current setting, chart coordinates are stored as SR variables; but since SymPy can be used as the symbolic backend on manifolds, via
chart coordinates could be stored as SymPy variables, instead of SR ones. |
comment:3
Thanks very much for the explanation and analysis! Replying to @egourgoulhon:
I have created #30065 for this |
comment:4
Replying to @egourgoulhon:
Thanks, I'll try this. Actually, would it make sense (at least for simple cases) to have a calculus method that only uses Sage's rational functions? |
comment:5
In a related direction, would it be of interest to have a category of "algebraic" differential manifolds, as a differential geometry view on real varieties and semialgebraic sets (and their boundaries)? |
comment:6
Replying to @mkoeppe:
Yes, as soon as you don't have to take a square root, as for instance in evaluating the volume n-form on a pseudo-Riemannian manifold (aka Levi-Civita tensor). In particular, this would forbid the computation of the triple scalar product within spherical coordinates in the Euclidean 3-space. Actually, it should be relatively easy to add a new calculus method, in addition to the two ones already implemented (SR and SymPy), via the classes CalculusMethod and ChartFunction. |
comment:7
Replying to @mkoeppe:
Yes, I think so. |
comment:8
Replying to @egourgoulhon:
Thanks! I have created #30070 for this. |
comment:9
Replying to @egourgoulhon:
OK, I have created #30069 for the case of real algebraic manifolds. Does sage-manifolds currently implement manifolds with boundary, or is it planned to implement them? |
comment:10
Replying to @egourgoulhon:
Actually, for spherical coordinates, there is already some issue with rational functions at the level of the metric tensor itself, since the components of the Euclidean metric involve the sine function. |
comment:12
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
comment:16
Replying to @mkoeppe: Regarding the new ticket description, I would not say that the first three examples, namely |
This comment has been minimized.
This comment has been minimized.
comment:17
Of course, I agree. The summary already showed the categories of the spaces, but I've added two headers to emphasize this point. |
comment:18
Replying to @egourgoulhon:
I have created #30080 for this |
This comment has been minimized.
This comment has been minimized.
comment:21
For further speedups of |
comment:22
Replying to @mkoeppe:
Yes, actually in the current framework, any uninitialized component of a vector/tensor is considered to be zero. So in for i in fmodule.irange():
v = fmodule.element_class(fmodule)
- for j in fmodule.irange():
- v.set_comp(self)[j] = fmodule._ring.zero()
v.set_comp(self)[i] = fmodule._ring.one()
vl.append(v) This would make the number of calls to for i in self._fmodule.irange():
v = self._fmodule.linear_form()
- for j in self._fmodule.irange():
- v.set_comp(basis)[j] = 0
- v.set_comp(basis)[i] = 1
+ v.set_comp(basis)[i] = self._fmodule._ring.one()
vl.append(v) The above unnecessary initializations to zero have been implemented at the early stage of the manifolds project, when we were not certain to keep the storage convention of having uninitialized components to be zero. Given the heavy use of that convention now, it's pretty safe to remove these lines. |
comment:23
The changes suggested in comment:22 are implemented in the above branch. New commits:
|
comment:50
Can you please briefly summarize in the description what has actually been done and what the new benefits are? It is very difficult for me to piece that all together just from the comments and cross references to other tickets. Thanks! :) In general, I would wait until the dependent tickets are merged. However, I am a bad role model due to my impatience. |
comment:52
Replying to @mkoeppe:
Yes, indeed. |
comment:53
Replying to @mjungmath:
The code added in this ticket is described in comment:22 and comment:23. |
comment:54
Replying to @egourgoulhon:
Thanks! |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Reviewer: Matthias Koeppe |
comment:61
Patchbot not green. The latest branch of #30074 has not been merged yet. |
comment:62
That is not a real error (the other patchbot also came back green). You don't need to have all of the other branches merged in. |
comment:63
Replying to @tscrim:
It is a real error. Pyflakes is complaining about an unused import.
Why is that? Wouldn't that cause a merge conflict later on? |
comment:64
Replying to @mjungmath:
No, it is not. Pyflakes are not real errors (but they are good to clear).
There is no need to add unnecessary extra merge commits when there is no conflict or a need to have the latest branch. If later changes on the first branch are done to unrelated parts to the changes on this branch, there is no conflict. Addendum - The pyflakes is handled on the later commits from #30074. So changing that here would cause a merge conflict and resolve a problem already fixed. |
comment:65
Replying to @tscrim:
Okay, I see what you mean. Replying to @tscrim:
Mh. Sorry, I don't get it. The #30074 will be merged into develop first because of the dependence, right? Afterwards, this ticket will be merged. If this ticket consists of an older version than #30074, this would cause a merge conflict, wouldn't it? Or, at least, the pyflakes error would be added again. |
comment:66
Replying to @mjungmath:
No. The result will look like this:
where |
comment:67
Replying to @tscrim:
Ah, I see. This is because the commit in #30074 is more recent, right? |
comment:68
Replying to @mjungmath:
Yes, but it is still based off a common base commit. |
comment:69
Replying to @tscrim:
Ah well. Thank you for explaining this, and sorry for delaying the process. This is good to know for future commits of mine. |
Changed branch from public/manifolds/init_module_basis-30061 to |
The n-dimensional Euclidean space is available in Sage in many variants.
This ticket brings the speed of the most basic operation (constructing the space) of
EuclideanSpace
(from sage-manifolds) closer to that of the other variants.Spaces without scalar product:
Spaces with scalar product:
NB: It is not in the category
MetricSpaces
, and thus the element methodsdist
andabs
(?!) are missing... The methods__abs__
,norm
, anddot_product
are unrelated to the inner product matrix; onlyinner_product
usesinner_product_matrix
.With this ticket (and its dependencies #30065, #30074):
Some caching is happening too. The second time:
Scalar product without a space:
Depends on #30065
Depends on #30074
CC: @egourgoulhon @tscrim @nbruin @mwageringel @mjungmath
Component: geometry
Author: Eric Gourgoulhon
Branch/Commit:
3ce0c15
Reviewer: Matthias Koeppe
Issue created by migration from https://trac.sagemath.org/ticket/30061
The text was updated successfully, but these errors were encountered: