atcollab / at

Accelerator Toolbox
Apache License 2.0
48 stars 31 forks source link

Optimize atimplib #658

Closed swhite2401 closed 9 months ago

swhite2401 commented 10 months ago

This PR proposes a couple optimization/simplification of the C library for collective effects, a 10% speed-up is obtained in tracking simulations.

swhite2401 commented 10 months ago

@lcarver could you please run your benchmarking tests to check that everything is fine?

lcarver commented 10 months ago

While I have the numbers and plots to hand. Here is the new branch MPI comparison.

image

Some numbers: master branch, Nslices=300, Nparts_per_slice=2000, Nturns=10k 1 core = 1318.8 60 cores = 30.4

optimize_atimplib, Nslice=300, Nparts_per_slice=2000, Nturns=10k 1 core = 1317 60 cores = 28.6

I will check MWT and TMCI soon and verify correct behaviour

lfarv commented 10 months ago

@swhite2401 : while you are on that, on line 366:

int i,ib,it,is;

Could you remove the it variable which is not used but generates warnings in compilation ?

swhite2401 commented 9 months ago

Hello, so this is a bit more than just optimization in the end: -I have found a bug when changing Nslice or Nturns on the fly for collective effects elements, this is no fixed and I have added a dimension check on the turnhistory to prevent memory errors -Minor changes relating the warning from the windows compiler -Simplification a of weigthed average calculation of vbeam in the beam loading passmethods

I would suggest to double check this thoroughly. On my side everything looks OK but I haven't used these features as extensively as you.

lcarver commented 9 months ago

Hello, I run a single bunch case and compare vbeam with analytical. But for BLMode=Wake the single bunch at high current gets destroyed and particles are lost. On the master the same script runs and gives good agreement.

For PHASOR mode the same values run nicely, so not a physics problem.

lcarver commented 9 months ago

for BLMode.WAKE, the Vgen is continuously decreasing. The Vbeam computation is good but the Vgen loop is broken somehow. I continue to investigate

lcarver commented 9 months ago

All example files are running nicely. I am now launching MWT and TMCI (but I suspect they will be fine due to the fact that the turnhistory modifications only really impact multibunch)

lcarver commented 9 months ago

While testing I found an unrelated bug in wake_elements.py

    def set_normfactxy(self, ring):
        l0, _, _ = ring.get_optics(ring)
        self.NormFact[0] = 1/l0['beta'][0]
        self.NormFact[1] = 1/l0['beta'][1]

ring.get_optics(ring) is wrong, it should be either

ring.get_optics()

or

at.get_optics(ring)

lcarver commented 9 months ago

TMCI and MWT both fine.

swhite2401 commented 9 months ago

While testing I found an unrelated bug in wake_elements.py

    def set_normfactxy(self, ring):
        l0, _, _ = ring.get_optics(ring)
        self.NormFact[0] = 1/l0['beta'][0]
        self.NormFact[1] = 1/l0['beta'][1]

ring.get_optics(ring) is wrong, it should be either

ring.get_optics()

or

at.get_optics(ring)

Fixed

swhite2401 commented 9 months ago

@lcarver you need to re-approve before I can merge