Skip to content
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

Some performance improvements in the manifolds code #33110

Closed
spaghettisalat opened this issue Jan 2, 2022 · 13 comments
Closed

Some performance improvements in the manifolds code #33110

spaghettisalat opened this issue Jan 2, 2022 · 13 comments

Comments

@spaghettisalat
Copy link
Contributor

Changes include:

  • use the first Bianchi identity in the computation of the Riemann tensor
  • compute volume forms with contravariant indices only as needed
  • don't try to simplify trivial expressions consisting only of a single number or symbolic variable

Component: manifolds

Author: Marius Gerbershagen

Branch/Commit: 59691a8

Reviewer: Eric Gourgoulhon

Issue created by migration from https://trac.sagemath.org/ticket/33110

@egourgoulhon
Copy link
Member

comment:2

Thanks for this ticket! I've added it to the metaticket #30525 (fill free to do it yourself for a next ticket related to manifolds) and will take a look in the coming days.

@mkoeppe mkoeppe modified the milestones: sage-9.5, sage-9.6 Jan 10, 2022
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 11, 2022

Changed commit from 3f0eec0 to 59691a8

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 11, 2022

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

b53b570Merge branch 'public/manifolds-performance-improvements' of git://trac.sagemath.org/sage into Sage 9.5.rc0
59691a8#33110: reviewer tweak

@egourgoulhon
Copy link
Member

comment:5

This is very nice! Thanks a lot! I've just performed some minor tweaks (PEP 8 + adding you to AUTHOR fields) and pushed them in the above commit.

Waiting for the patchbot green light for positive review.

@egourgoulhon
Copy link
Member

Reviewer: Eric Gourgoulhon

@egourgoulhon
Copy link
Member

comment:6

Regarding the computation of the Riemann tensor, I've performed some benchmarks with Kerr, Kerr-Newmann and 5D Kerr-AdS metrics, via these notebooks: 1, 2 and 3 respectively. I've have not noticed any significant speed-up. On your side, do you have any example with some net speed-up?

@egourgoulhon
Copy link
Member

comment:7

On the other side, I've noticed a tremendous speed-up in the vector field plots, due to the treatment of expressions with a single term in simplifying functions. This is very welcome, until vector field plots are improved by using fast callable expressions (this will require a full rewriting of VectorField.plot, to make it not depend on TangentVector.plot, in some future ticket).

@spaghettisalat
Copy link
Contributor Author

comment:8

Replying to @egourgoulhon:

Regarding the computation of the Riemann tensor, I've performed some benchmarks with Kerr, Kerr-Newmann and 5D Kerr-AdS metrics, via these notebooks: 1, 2 and 3 respectively. I've have not noticed any significant speed-up. On your side, do you have any example with some net speed-up?

I find a small speedup (about 3%) for the Kerr metric in the single-threaded case. The multi-threaded case is a little bit slower for me, but that is most certainly because some of the cores are inactive for a little longer due to the naive way that sage distributes the computation across multiple cores. One should really measure CPU usage instead of total time spent.

But in general my changes reduce the number of components that have to be calculated using contractions of the connection from n2(n-1)/2 to n(n2-1)/3 which in the best case for large n could be up to 33% faster. Of course, if the computation time is dominated by the final simplification step after the contraction (or subtraction when using the Bianchi-identity) has been performed, then there won't be much of a speedup. I suspect that this is the case in your examples which have quite complicated Riemann tensors.

@egourgoulhon
Copy link
Member

comment:9

Replying to @spaghettisalat:

Thanks for your answer.

Of course, if the computation time is dominated by the final simplification step after the contraction (or subtraction when using the Bianchi-identity) has been performed, then there won't be much of a speedup. I suspect that this is the case in your examples which have quite complicated Riemann tensors.

Yes. Most probably, in the case I've considered the computation time is dominated by the gam_gam term, which involves multiplication of connection coefficients and which is computed for the full range of indices, irrespective of the Bianchi identity.

@egourgoulhon
Copy link
Member

comment:10

Replying to @egourgoulhon:

Waiting for the patchbot green light for positive review.

Well after 2 weeks, the patchbot has not shown up... I am setting the ticket to positive review without waiting any further, since all doctests are passed on my computer and the doc builds (both html and pdf).

@egourgoulhon
Copy link
Member

comment:11

The ticket branch has not been merged in Sage 9.6.beta1. Maybe it is missing some patchbot green light. So I'm setting the status back to "needs_review" in order to catch the patchbot's attention.

@tscrim
Copy link
Collaborator

tscrim commented Feb 14, 2022

comment:12

There is some order that the tickets get merged in, but I don't quite understand what that is.

@vbraun
Copy link
Member

vbraun commented Feb 16, 2022

Changed branch from public/manifolds-performance-improvements to 59691a8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants