-
Notifications
You must be signed in to change notification settings - Fork 104
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
Performance improvements for MW orbit integration #701
Comments
Hi @jamesgrimmett, Thanks for looking into this, these are great optimizations! Some comments:
|
A more general solution along the lines that you suggest for the caching of the force components would also be great, because it would solve all spherical potentials and the ellipsoidal ones, but it would be a lot more work, so just doing the caching for |
Great, thanks for the comments @jobovy. Interesting, I'll take a look at calling the Q function directly, and compare with my results using the P function. I'll open a PR for this sometime this week, hopefully it should be a quick one. That's very useful that there is already an example for caching results! That looks to be more straightforward than I initially imagined, I'll also have a go at a PR for the caching machanism for |
Hi @jobovy, I have recently been looking into whether I could improve the runtime for integrating orbits when using the
MWPotential2014
potential. (I'm trying to model up to 1000 orbits on a small number of cpus, so any speedup is useful).I've noted two things that could help to reduce runtime;
(i) In the force calculations in
PowerSphericalPotentialwCutoff.c
, the interior mass is found usingi.e.,
I've found that a fair amount of the runtime is spent in
gsl_sf_gamma_inc
. A cheaper way to compute the lower integral is withi.e.,
I don't completely understand why$x, a$ (i.e., $alpha, R$ ).
gsl_sf_gamma_inc_P
is signficantly faster thangsl_sf_gamma_inc
, to be honest. I think it is to do with GSL's internal optimisations for the lower integral vs. the upper unbounded integral. The speedup also depends on the value ofIn practice (on 12 cpus) I find that this change reduces
Orbit().integrate()
runtime by 40%, andOrbit.from_name('MW globular clusters').integrate()
runtime by 85% (over a 10Gyr integration period).(ii) As the Rforce and zforce computations tend to be very similar, there are some redundant calculations made during each call to
evalRectForce
. E.g., for aPowerSphericalPotentialwCutoff
potential, the interior mass is calculated once for the Rforce and again for the zforce (see screenshot below showing a section of the call map,gsl_sf_gamma_inc
is proxy for the mass calculation, percentages indicate the amount of time that each function was active during runtime (% relative to calling function total)).I am wondering if there is a good way to pull out at least the mass calculation (if not the other components as well) from the individual force calculations, while still retaining the ability to compute each force in a standalone way. E.g., perhaps something along the lines of an additional "gravforce" function (unsure about appropriate function name);
So that from within
evalRectForce
we could havegravforce = calcgravForce(); Rforce = R * gravforce; zforce = z * gravforce
, saving computing the interior mass twice.I haven't had a close look at how this restructuring would be applied to the force calculations in other potentials as well, but I imagine the speedup will depend on how expensive+similar the R and z force calculations are for each.
Would you be keen for these changes to be brought into
main
? If so, I'd be happy to open a PR with the changes for at least (i). I'm also happy to include the changes for (ii), though I may need some advice on the best approach.The text was updated successfully, but these errors were encountered: