Closed mkoeppe closed 4 years ago
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.
Replying to @mjungmath:
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! :)
The code added in this ticket is described in comment:22 and comment:23. The other code in the ticket branch arises from the dependencies tickets #30065 and #30074.
Replying to @egourgoulhon:
The code added in this ticket is described in comment:22 and comment:23. The other code in the ticket branch arises from the dependencies tickets #30065 and #30074.
Thanks!
Description changed:
---
+++
@@ -134,6 +134,22 @@
(timeout)
+With this ticket (and its dependencies):
+
+ +sage: %time EuclideanSpace(5) +CPU times: user 4.87 ms, sys: 372 µs, total: 5.24 ms +Wall time: 4.93 ms +5-dimensional Euclidean space E^5 +sage: %time EuclideanSpace(80) +CPU times: user 106 ms, sys: 4.52 ms, total: 110 ms +Wall time: 92 ms +80-dimensional Euclidean space E^80 +sage: %time EuclideanSpace(1000) +CPU times: user 1.04 s, sys: 33.5 ms, total: 1.07 s +Wall time: 986 ms +
+
Scalar product without a space:
Description changed:
---
+++
@@ -1,4 +1,6 @@
-The n-dimensional Euclidean space is available in Sage in many variants. We investigate the speed of the most basic operation: Constructing the space.
+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:**
Description changed:
---
+++
@@ -151,7 +151,14 @@
CPU times: user 1.04 s, sys: 33.5 ms, total: 1.07 s
Wall time: 986 ms
+Some caching is happening too. The second time:
+ +sage: %time EuclideanSpace(1000) +CPU times: user 207 ms, sys: 5.63 ms, total: 213 ms +Wall time: 212 ms +1000-dimensional Euclidean space E^1000 +
Scalar product without a space:
Description changed:
---
+++
@@ -136,7 +136,7 @@
(timeout)
-With this ticket (and its dependencies): +With this ticket (and its dependencies #30065, #30074):
sage: %time EuclideanSpace(5)
Reviewer: Matthias Koeppe
Patchbot not green. The latest branch of #30074 has not been merged yet.
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.
Replying to @tscrim:
That is not a real error (the other patchbot also came back green).
It is a real error. Pyflakes is complaining about an unused import.
You don't need to have all of the other branches merged in.
Why is that? Wouldn't that cause a merge conflict later on?
Replying to @mjungmath:
Replying to @tscrim:
That is not a real error (the other patchbot also came back green).
It is a real error. Pyflakes is complaining about an unused import.
No, it is not. Pyflakes are not real errors (but they are good to clear).
You don't need to have all of the other branches merged in.
Why is that? Wouldn't that cause in a merge conflict later on?
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.
Replying to @tscrim:
Replying to @mjungmath:
Replying to @tscrim:
That is not a real error (the other patchbot also came back green).
It is a real error. Pyflakes is complaining about an unused import.
No, it is not. Pyflakes are not real errors (but they are good to clear).
Okay, I see what you mean.
Replying to @tscrim:
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.
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.
Replying to @mjungmath:
Replying to @tscrim:
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.
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.
No. The result will look like this:
B--0--X--T----M
\ /
0--0--H
where T
is this branch and H
is the head of #30074. So the #30074 branch will be merged first (all commits marked 0
starting from the latest beta B
and H
), then the commits from this branch (labelled X
and T
), which will result in the merge commit M
. This would be exactly the same as if you merged in the latest commits from #30074 because that would be doing the same thing. The only difference is you have an extra merge commit in where this branch is pointing to.
Replying to @tscrim:
No. The result will look like this:
B--0--X--T----M \ / 0--0--H
where
T
is this branch andH
is the head of #30074. So the #30074 branch will be merged first (all commits marked0
starting from the latest betaB
andH
), then the commits from this branch (labelledX
andT
), which will result in the merge commitM
. This would be exactly the same as if you merged in the latest commits from #30074 because that would be doing the same thing. The only difference is you have an extra merge commit in where this branch is pointing to.
Ah, I see. This is because the commit in #30074 is more recent, right?
Replying to @mjungmath:
Ah, I see. This is because the commit in #30074 is more recent, right?
Yes, but it is still based off a common base commit.
Replying to @tscrim:
Replying to @mjungmath:
Ah, I see. This is because the commit in #30074 is more recent, right?
Yes, but it is still based off a common base commit.
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 3ce0c15
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