-
Notifications
You must be signed in to change notification settings - Fork 29
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
Multibunch Beamloading #516
Conversation
for e in self: | ||
if e.is_collective: | ||
e.clear_history(ring=self) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This assumes that all Collective
elements have a clear_history()
method. This could be enforced by adding an @abstact_method
to the Collective
class. I propose an another PR for that, this one is large enough!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right! Maybe put an issue as a reminder? Or we just add it here it is a small addition
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you like !
@swhite2401 and @carmignani : could you both confirm that this PR supersedes #191, and that that #191 could be closed? |
About remove mail adresses from C files: it would have been preferable to do this in another PR: you are mixing different things and increasing a lot the number of modified files ! For now, keep it here, of course! |
@@ -90,7 +90,7 @@ function orbit6(testCase,lat2) | |||
porbit6=double(porbit6)'; | |||
% Matlab | |||
[~,morbit6]=findorbit6(lattice.m); | |||
testCase.verifyEqual(morbit6,porbit6,AbsTol=1.E-15 ); | |||
testCase.verifyEqual(morbit6,porbit6,AbsTol=5.E-15 ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did anything changed in the computation of the closed orbit? Do you get problems with the previous value?
I don't care about 1 or 5 e-15, but that may indicate that something unexpectedly changed…
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not change anything but the matlab test was systematically failing so I just matched the tolerance of the matlab test to the one of the python test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Failing on GitHub ? It still succeeds on the other branches, so it looks strange…
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So there is one thing. energy_loss.py
was changed and is used to determine the initial condition for find_orbit6
. This could have an impact on the final result to that level no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, that could be an explanation. Do you confirm that it fails on GitHub (which platform?), because it still works for me (MacOS).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found the last failure in the GitHub history: it failed on ubuntu-latest, with an error of 3.2e-15 instead of 1e-15. We don't have the results on the other platforms because all runs are cancelled as soon as one fails.
So let's assume energy_loss.py
is the reason for the change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The matlab tests fail on GitHub, I could not install the matlabengine in my pyenv, either it complains about the matlab version or the python version so I gave up.
All the functionalities of #191 are integrated in this one, so yes it can be closed |
Yes sorry, I realized after that this was a bit bold...anyway now it is done |
…into beamloading_iterative
@swhite2401 : another question: why do you implement you own turn counter in the integrator? This prevents you from stopping/restarting the tracking, and it may lead to different turn numbers depending on the integrator. Also, by relying upon the "standard" turn counter, you could clear the turn history on turn zero in the integrator, avoiding the |
Well, I was facing issues with memory allocation. The length of the position and size vector are fixing in lattice_pass bu the argument Concerning the clear history, you may not necessarily want to clear it for a new tracking...maybe the oppsoite in fact to start from steady state. For me it is more relevant when you change beam parameters like the |
So I have already approved these changes. but I did some further tests following comments from @carmignani and @lfarv. Below are some quick plots showing some expected effects.
|
@lcarver : thanks for the checks: very nice, it's convincing! |
The multi-bunch beam loading is introduced. Several additions are present in this PR:
RFCavityBeamloadingPass.c
extenting the functionalities proposed in Cavity with beam loading #191 with multibunch, MPI, a 2 energy loss calculation methodscollective
subpackage andlattice_object.py
The proposed strategy is to modify the cavity elements in-place to integrate the beam loading, all the calculations are the done in the C tracking engine. Similarly beam loading can be removed from cavities.
The energy loss in the cavity can be calculated using the phasor method or the wake function, both are compatible with MPI and multibunch tracking.
A feedback loop is integrated and can be tuned using the
*Gain
arguments, this allows to maintain the effective voltage and cavity phase constant using the tuning angle and generator amplitude regardless of the beam induced voltage. This feedback is presently ideal but can be further extended to integrate more realistic circuits.This implementation was benchmarked using
mbtrack
and theory.