Deprecate `taylorinteg_threads`

PerezHz / PlanetaryEphemeris.jl

Solar System ephemeris Taylor integrator based on JPL DE430/431 dynamical model

Other

20 stars 6 forks source link

Deprecate `taylorinteg_threads` #25

Closed LuEdRaMo closed 1 year ago

LuEdRaMo commented 1 year ago

In order to solve https://github.com/PerezHz/PlanetaryEphemeris.jl/issues/20, this PR deprecates taylorinteg_threads in favor of taylorinteg. As a reference, a 100 steps, 4 threads integration gives the followinig benchmarks:

main:

-----------------
Integrator warmup
-----------------
74.505484 seconds (30.99 M allocations: 6.915 GiB, 4.37% gc time, 299.02% compilation time)
----------------
Full integration
----------------
720.649193 seconds (677.46 M allocations: 169.086 GiB, 3.20% gc time)

this PR:

-----------------
Integrator warmup
-----------------
74.704210 seconds (30.97 M allocations: 6.914 GiB, 4.33% gc time, 300.24% compilation time)
----------------
Full integration
----------------
699.725729 seconds (677.46 M allocations: 169.083 GiB, 1.97% gc time)

Also, results do not change between both branches.

PerezHz commented 1 year ago

Many thanks for this, @LuEdRaMo! I think this will help us avoid maintaining duplicate code, while reusing taylorinteg. Thank you also for posting the benchmarks, those are indeed helpful. It makes me wonder, have you tried doing runs, of say, about 10 years, in many threads, say 40 threads? I think that could help us have a better idea of the difference in performance between current main and this PR.

LuEdRaMo commented 1 year ago

A 10 years, 40 threads integration (in Julia v1.9.2) gives the following benchmarks:

main:

-----------------
Integrator warmup
-----------------
102.183042 seconds (31.20 M allocations: 6.942 GiB, 2.73% gc time, 3707.83% compilation time)
----------------
Full integration
----------------
2687.964587 seconds (10.67 G allocations: 2.631 TiB, 17.19% gc time)

this PR:

-----------------
Integrator warmup
-----------------
100.689653 seconds (31.19 M allocations: 6.940 GiB, 1.87% gc time, 3751.48% compilation time)
----------------
Full integration
----------------
2561.300486 seconds (10.67 G allocations: 2.631 TiB, 17.47% gc time)

Also, results do not change between both branches.

PerezHz commented 1 year ago

Many thanks for checking this! From this I reckon performance is pretty much the same actually... These runs include all the 343 asteroids, right?

LuEdRaMo commented 1 year ago

These runs include all the 343 asteroids, right?

Yes, the benchmarks correspond to a full Solar System integration.

PerezHz commented 1 year ago

Alright, then just let me know when this is ready for review.

LuEdRaMo commented 1 year ago

I think this is ready for review.

PerezHz commented 1 year ago

I'm thinking that this can be an opportunity for us to get rid of NBP_pN_A_J23E_J23M_J2S!, since in practice we only use the threaded version in DE430!. Can you try removing this function and the associated jetcoeffs! method?

PerezHz commented 1 year ago

Thanks for taking care of this; merging!