atcollab / at

Accelerator Toolbox
Apache License 2.0
48 stars 31 forks source link

Add passive beamloading cavity #586

Closed lcarver closed 10 months ago

lcarver commented 1 year ago

Modified the existing functions to allow the user to specifiy a passive cavity. In this situation, Vgen is always zero, no RF cavity is tracked, only the vbeam option is tracked and applied to the particles.

A new enumerator was provided called CavityMode and CavityMode.ACTIVE is the default.

It seems to be working well, but I don't have yet a good case to benchmark. So I have marked it as a WIP for the time being

lcarver commented 11 months ago

Following the nice benchmarking results made by Teresia at the AT Workshop using this branch (slides are here and the good agreement pasted below: https://indico.esrf.fr/event/93/contributions/518/ )

I remove the WIP label and request review.

image

swhite2401 commented 11 months ago

@lcarver, could you please update this branch with the master before review? The label WIP is still there too

lcarver commented 11 months ago

@lcarver, could you please update this branch with the master before review? The label WIP is still there too

done

lcarver commented 11 months ago

ready for re-review (and welcome to @TeresiaOlsson to the reviewing side of AT).

I implemented the changes, now in the beamloading passmethod, if vgen is 0 and cav_length is 0 it skips the loop which means it can be in the main code irrespective of active or passive. I added some checks to makes sure for a passive cavity vgen setpoint is always 0.

lcarver commented 11 months ago

I have one other question to ask before I implement the changes (in case it's not needed).

At the moment I have to do mode=BLMode.WAKE, cavitymode=CavityMode.ACTIVE

should 'mode' be renamed 'blmode' in order to be clearer?

lcarver commented 11 months ago

ready for review / merge

lfarv commented 11 months ago

I'll trust the experts…

lcarver commented 11 months ago

had to remerge master, i can't keep up! also checked example files.

lfarv commented 11 months ago

had to remerge master, i can't keep up!

There is no need to merge master unless there is a conflict, which is not the case here. It just makes the commit history longer…

lcarver commented 11 months ago

changes implemented. Ready for re-review

lcarver commented 11 months ago

@swhite2401 changes implemented

swhite2401 commented 11 months ago

I all benchmarks are OK this is fine for me!

lcarver commented 11 months ago

my local at directory is a bit of a mess right now as running checks for optimize_atimplib at the same time. I propose to wait until I am back next week, finalise some clean tests on this branch and then merge.

swhite2401 commented 11 months ago

Yes ok better wait than merging something incorrect

lcarver commented 10 months ago

I reran one of the MAXIV cases shown by Teresia (173 bunches, Nslice=50, 10k particles per bunch, 150k turns) and I was able to perfectly reproduce her results. So I am happy and will now merge.