Fix bug in memory allocation of wake method

atcollab / at

Accelerator Toolbox

Apache License 2.0

48 stars 31 forks source link

Fix bug in memory allocation of wake method #579

Closed lcarver closed 1 year ago

lcarver commented 1 year ago

double memory allocation of wake (2 numbers) led to runaway memory. Surprised it wasn't spotted before. Bug is now fixed by using pointer instead of extra allocation

swhite2401 commented 1 year ago

Ok for me. There are most likely cleaner solutions but at least this one fixes the bug

lcarver commented 1 year ago

So in the latest version of this PR. Memory leak problem is solved. More accurate method of computing the Vbeam is now put in place. Now it computes (correctly!) the weighted mean of the voltage kick experienced by the beam. Previously I was seeing a few strange things, such as noisy Vbeam value turn by turn, which didn't converge with increasing nslice and nparts. This should now be resolved.

Before I merge I would like to redo the benchmarking for the usual things (Qs, transient and checking Vbeam with analytical values)

lcarver commented 1 year ago

Full test of all different cases, comparing the Vbeam for a range of currents (with and without zcuts, for phasor and for wake, for Nslice=1 or Nslice>1) have all passed. Agreement is excellent. Many more cases were tested, all agreeing well, the cases with zcuts requires a bit of numerical optimization but that's to be expected. As an example for what I was comparing, see the two cases below.

For Nslice=1 and Nbunches=16. Note that in this case, with very few particles per bunch (20 as I was running on 20 cores), the wake model was going unstable. I am certain that it is due to transients that I could remove with more time invested.

lcarver commented 1 year ago

For me this is now ready to merge, I am running the benchmark for Nslice=50 for Qs vs current but to be honest, the Vbeam agrees perfectly so there is no reason it should not pass.

I leave it a few days anyway before I merge in case someone wants to comment. I will probably merge on friday morning (after Nslice=50 test has passed)

lfarv commented 1 year ago

Hi @lcarver. Could you declare all functions in atimplib.c as "static". It avoids cluttering the namespace (and prevents name collisions).

lcarver commented 1 year ago

Hi @lcarver. Could you declare all functions in atimplib.c as "static". It avoids cluttering the namespace (and prevents name collisions).

All your suggestions have been implemented. I push the merge date back until Monday, as I did not run properly the longer benchmarking.

lcarver commented 1 year ago

Above plot for slightly higher Rs (13 cavs instead of 11 - explains different shift rate and threshold) and done for Nslice per bunch = 50.

All good for me.