Skip to content
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

Set the default cavity PassMethod to RFCavityPass #372

Merged
merged 1 commit into from
Apr 1, 2022
Merged

Conversation

lfarv
Copy link
Contributor

@lfarv lfarv commented Feb 19, 2022

The legacy CavityPass pass method is wrong when tracking for more than one turn, if the cavity frequency is different from the nominal frequency. RFCavityPass is correct, but is not the default one applied at element creation or when turning radiation ON.

This pull request makes RFCavityPass the default pass method for cavities, in both Matlab and python versions.

CavityPass should not be used any more. It remains available for compatibility with lattices using it.

@lfarv lfarv added Matlab For Matlab/Octave AT code Python For python AT code bug fix labels Feb 19, 2022
@swhite2401
Copy link
Contributor

@lfarv, ok for me to merge as this has been discussed at length already

@lfarv lfarv requested a review from lnadolski February 28, 2022 12:46
@simoneliuzzo
Copy link
Contributor

Dear @lfarv, @carmignani and @swhite2401 ,

the tracking is unaffected by this change?
I remember that RFCavity pass would change the ct coordinate. May be I am wrong.

best regards
Simone

@swhite2401
Copy link
Contributor

@simoneliuzzo , you are right the 5th coordinate will drift away but only if the cavity frequency is not nominal and only after the first turn

@simoneliuzzo
Copy link
Contributor

Let's say I want to do a classic ct - delta phase space picture. Will I be able to obtain the same results as before (ex, the club shape)? Could you provide a script/figure showing that this is the case, thus proving that the change is transparent?

@lfarv
Copy link
Contributor Author

lfarv commented Mar 23, 2022

@simoneliuzzo : As previously mentioned by @swhite2401, if you are on the nominal frequency, the results are exactly the same. If not, CavityPass is wrong, so the phase-space picture you get with it is completely wrong. It's always better to use RFCavityPass. CavityPass will be kept for compatibility, but it's wrong for non-nominal RF frequency.

@simoneliuzzo
Copy link
Contributor

I would like to approve this pull request by seeing the effect of the change. Could someone provide those figures (phase space before/after, 4D/6D, nominal/varied frequency)? Else I will do it, but I honestly do not know when.
Is there (or will there be) a test (at least in python) making sure the tracking is not changing when changing the default integrators?

@swhite2401
Copy link
Contributor

@simoneliuzzo I will produce the requested plots, for reference you may look at the discussion in #317

@swhite2401
Copy link
Contributor

@simoneliuzzo , here is the tracking your requested:
-nominal frequency -> identical in all planes
-dp=0.01 (frequency shifted) -> identical x, y + ct coordinate drifting

To conclude: CavityPass behavior is reproduced for the nominal frequency. With offset frequency CavityPass was wrong as it would always assume the nominal frequency, this is solved by this PR: results are different for offset frequency but this is expected as they wrong before

image

@swhite2401
Copy link
Contributor

If needed you may remove the drift by subtracting the trajectory of the reference particle:

image

@lfarv
Copy link
Contributor Author

lfarv commented Mar 31, 2022

If you think that tracking the closed orbit is a waste of resources, you can also remove the closed orbit drift analytically: the drift per turn is exactly "β.c.h/fRF - circumference". Which gives 0 for the nominal frequency!

β: relativistic beta
c: velocity of light
fRF: RF frequency
h: harmonic number

@simoneliuzzo: are you convinced?

@simoneliuzzo
Copy link
Contributor

I think any user presently exploiting the ct coordinate as tracked using CavityPass will see his/her scripts returning a different result once this branch is merged.

I think the plot is rather convincing about the fact that we gain something, but we lose something else.

Seen the tracking of Simon, a ct-delta phase space plot is changing with the new AT defaults, unless some action is taken by the user. Could we include the ct correction in ringpass/linepass for example?

I will not stop from merging this branch, but I think it will lead to issues in the future.

@lfarv
Copy link
Contributor Author

lfarv commented Mar 31, 2022

Seen the tracking of Simon, a ct-delta phase space plot is changing with the new AT defaults, unless some action is taken by the user.

Action ? What kind of action ? Don't do anything !

I show you in the next comment how CavityPass is completely wrong.

@lfarv
Copy link
Contributor Author

lfarv commented Mar 31, 2022

Let's try to track the off-momentum closed orbit with CavityPass and RFCavityPass:

import at
import numpy as np
import matplotlib.pyplot as plt
plt.rcParams["figure.figsize"] = (9.0, 6.0)

Take the hmba test lattice:

ring0=at.load_lattice('/Users/famille/dev/libraries/at/pyat/machine_data/hmba.mat').radiation_off(copy=True)
allpts=range(len(ring0)+1)

Set the PassMethod of ring1 to CavityPass and the PassMethod of ring2 to RFCavityPass

ring1=ring0.radiation_on(cavity_pass='CavityPass',dipole_pass=None, quadrupole_pass=None, copy=True)
ring2=ring0.radiation_on(cavity_pass='RFCavityPass',dipole_pass=None, quadrupole_pass=None, copy=True)

Set the frequency to get 0.1% off-momentum:

dp=0.001
ring1.set_rf_frequency(dp=dp)
ring2.set_rf_frequency(dp=dp)

Compute the off-momentum closed orbit of both lattices:

o1,orbs=at.find_orbit6(ring1)
o2,orbs=at.find_orbit6(ring2)
print(np.stack((o1,o2), axis=0).T)
[[1.75602735e-06 1.75602735e-06]
 [4.01623958e-12 4.01623958e-12]
 [0.00000000e+00 0.00000000e+00]
 [0.00000000e+00 0.00000000e+00]
 [9.98181393e-04 9.98181393e-04]
 [8.89596672e-17 8.89596672e-17]]

As expected the off-momentum closed orbits are identical

Now, track the closed orbit of both lattices

nturns=8000
rout1=at.lattice_pass(ring1,o1.copy(),nturns)
rout2=at.lattice_pass(ring2,o2.copy(),nturns)

Plot both results

nplot=8000
step=50

line1=plt.plot(rout1[5,0,0,:nplot:step],rout1[4,0,0,:nplot:step],'.',label='CavityPass')
line2=plt.plot(rout2[5,0,0,:nplot:step],rout2[4,0,0,:nplot:step],'.',label='RFCavityPass')
plt.xlabel('${\Delta}s$')
plt.ylabel('$\delta$')
plt.legend()

png

As you can see on the plot, with CavityPass, the closed orbit is not closed at all: it oscillates around the on-momentum closed orbit (δ=0, ct=0). As if the RF frequency was nominal.

  • RFCavityPass: δ stays constant at 0.001: OK

  • CavityPass: the "closed orbit" oscillates around the on-momentum closed orbit: WRONG

    The frequency shift is ignored by CavityPass

@lfarv
Copy link
Contributor Author

lfarv commented Mar 31, 2022

Conclusion:

NEVER USE CavityPass WITH SHIFTED RF FREQUENCY: IT IS IGNORED

@simoneliuzzo : do you think that knowing this, we can keep CavityPass as default ? Obviously not !

I think any user presently exploiting the ct coordinate as tracked using CavityPass will see his/her scripts returning a different result once this branch is merged.

Of course they will ! The bug is corrected. And it's a crucial one.

Could we include the ct correction in ringpass/linepass for example?

What do you want to correct: what you get is the correct result! For a lower RF frequency, the path length is longer, so the delay with respect to the reference increases turn after turn on the average (monotonously for the closed orbit, with oscillations for other trajectories).

I will not stop from merging this branch, but I think it will lead to issues in the future.

I do not see how correcting bugs may lead to issues. This bug was pointed out and corrected by @carmignani more that 5 years ago, it's too bad that we waited until now to set the correction as the default.

@swhite2401
Copy link
Contributor

@lfarv, while I agree with you that when fixing a bug we cannot expect identically wrong results, I think @simoneliuzzo has a point concerning the functionality to remove the ct drift. I would personally very much appreciate such feature.

It is simple enough to be added as an optional feature in lattice_pass so I do not understand why you are so much against it. We already had this discussion: there is nothing wrong in changing the reference frame and if users want it why blocking it?

@lfarv
Copy link
Contributor Author

lfarv commented Apr 1, 2022

Also note that because of the energy variation, the horizontal plane is also affected:
Here is the horizontal phase-space, in a dispersive region, of the off-momentum 6D closed orbit of the previous example:

nplot=8000
step=50

line1=plt.plot(rout1[0,0,1,:nplot:step],rout1[1,0,1,:nplot:step],'.',label='CavityPass')
line2=plt.plot(rout2[0,0,1,:nplot:step],rout2[1,0,1,:nplot:step],'.',label='RFCavityPass')
plt.xlabel('$x$')
plt.ylabel('$x^{\prime}$')
plt.legend()

png

Again, with RFCavityPass, the closed orbit is fixed (a single point), while with CavityPass it moves back and forth.

@lfarv
Copy link
Contributor Author

lfarv commented Apr 1, 2022

@swhite2401, @simoneliuzzo :

Let's try no to mix two things:

I'd like to merge this one and make a new release once it's done. Do we agree that CavityPass must be eliminated (as the defaults, at least) ?

For the 2nd point, we can open a new discussion as a follow-up of #317, though I hoped it was cleared once far all…

@swhite2401
Copy link
Contributor

I have nothing against this PR and approved it already.
I am just expressing an opinion that I have already expressed in the past, it was not cleared at all: a functionality to remove the ct drift is highly desirable, it is not wrong (you even gave the formula!!) and the user should be given the opporunity to apply it if he wants to

@lfarv lfarv merged commit 7aa0e26 into master Apr 1, 2022
@lfarv lfarv deleted the cavity_passmethod branch April 1, 2022 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug fix Matlab For Matlab/Octave AT code Python For python AT code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants