atcollab / at

Accelerator Toolbox
Apache License 2.0
48 stars 31 forks source link

beamloading set point not correct for simple_ring #694

Closed lcarver closed 7 months ago

lcarver commented 8 months ago

Hello, phi_s is not set correctly when adding beam loading with a simple_ring. Example below:

import numpy as np
from at import simple_ring
import at
from at import add_beamloading, 

Vc = 5.5e6

ring_full = at.load_mat('../Repo/pyce/machine_data/esrf.mat', mat_key='LOW_EMIT_RING_INJ')
ring_full.set_rf_voltage(Vc)
ring_full.set_cavity_phase()
_, ring_pyat = at.fast_ring(ring_full)

U0 = ring_full.get_energy_loss()

ring_simple = simple_ring(ring_full.energy, ring_full.circumference, ring_full.harmonic_number, 0.1, 0.1, Vc, at.get_mcf(ring_full.disable_6d(copy=True)), U0=U0, TimeLag=100)

add_beamloading(ring_pyat, 100, 1, cavpts=[0], Nslice=50, VoltGain=1e-3, PhaseGain=1e-3)
print(ring_pyat[0]._phis)

add_beamloading(ring_simple, 100, 1, cavpts=[0], Nslice=50, VoltGain=1e-3, PhaseGain=1e-3)
print(ring_simple[0]._phis)

Output:

0.47833528
0.0

No matter what I put for TimeLag in simple_ring, the phi_s is always zero.

In beam_loading.py, we can see in line 188, in the function add_beamloading _, ts = get_timelag_fromU0(ring)

This value of ts is not computed correctly for a simple_ring. Whatever I put for timelag, ts comes out to be -TimeLag which always gives a phi_s of 0.

lcarver commented 8 months ago

in get_energy_loss, the fast_ring lattice returns the correct U0 and therefore the correct timelag simple_ring returns energy_loss of 0

lcarver commented 8 months ago

So I know the problem, but no good idea of the solution.

To compute the U0, you must give get_energy_loss a lattice with rad on (for ELossMethod = TRACKING). It then switches off all collective, diffusion and cavity elements and tracks, element by element and computing sum of all energy changes.

For simple_ring, these energy changes always come out to be 0, as the energy loss is handled in the simple quantum diffusion element. I'm also not entirely sure if, when I added SimpleQuantDiff, if it was properly added to the is_collective group.

There is some bug here.

lcarver commented 8 months ago

Also as a separate smaller and stranger bug

if I take my ring_simple and run (like in the get_energy_loss function) ringtmp = ring.disable_6d(at.RFCavity, at.QuantumDiffusion, at.Collective, copy=True)

then no errors, but then (for the simple quantdiff pass)

at.internal_lpass([ringtmp[3]], np.zeros(6))

fails with error:

AttributeError: list has no attribute replace

while all the other elements work well (returning zeros)

swhite2401 commented 8 months ago

Hi @lcarver ,

This is all (almost) normal behavior: -quantum diffusion has to be disabled to get U0 otherwise you get variable results -internal_lpass is for internal use only: do not use it elsewhere, there is a user tracking interface for this

So the only issue there is that the energy loss is mixed with quantum diffusion. I can see 2 options: 1-You separate the quantum diffusion from the rest, which is what we decided not to do... 2-You allow for U0 to be an input argument for the functions causing problems

I still like to have a single element for all radiations effects...but we can reconsider this if it is more practical... Option 2 would require changes in add_beamloading, BeamLoadingElement and getTimeLagfromU0 such that U0 is accepted as an optional argument

QuantDiff is NOT collective, and should not be! In fact I do not understand why the command above disables it...the name is also not really appropriate because it does much more than quantum diffusion... this has to be reviewed

lcarver commented 8 months ago

Hi, thanks. I was a bit mistaken before ( I think). The disable_6d in get_energy_loss is working well. The simplequantdiff element is switched off, so it returns 0 as a change in dp. i was confusing 'collective' with 'diffusion'. My bad.

I could also add a line in get_energy_loss / tracking function

u0_diffusion_elems = numpy.sum([e.U0 for e in ringtmp if hasattr(e,'U0'])

This way if an element has U0 then it is added, but I think that is not acceptable and starts us on a bad course for the future.

lcarver commented 8 months ago

On a separate note I have some issues with U0 computation in fast ring. The U0 with the S28F lattice is

ring_full = at.load_mat('../Repo/pyce/machine_data/esrf.mat', mat_key='LOW_EMIT_RING_INJ')
_, ring_pyat = at.fast_ring(ring_full)

print('fullring', ring_full.get_energy_loss(method=at.ELossMethod.TRACKING))
print('fastring', at.get_energy_loss(ring_pyat, method=at.ELossMethod.TRACKING))

Output:

fullring 2531662.0976...
fastring 2531660.675...

Problem comes from M66. I am investigating, and will make a PR

swhite2401 commented 8 months ago

How is simeplequantdiff switched off? with the command above? It is neither a collective of a quantumdiffusion element... or maybe I am missing something? I agree what you propose is not nice... there are cleaner ways to handle this

You are not comparing the results of the same method, you should use tracking for both

lfarv commented 8 months ago

Hello @lcarver and @swhite2401. I agree with the diagnosis of @swhite2401 : the problem comes from the fact that all 3 radiation effects (damping, energy loss and quantum diffusion) are grouped in the same element. The splitting was discussed in #643 and ended in this design decision. I still think that separating the quantum diffusion is better. @swhite2401's option 2 introduces unnecessary complications and is really inelegant. Looking at atpass.c, you can see that having another element cost nothing:

    for (elem_index = 0; elem_index < num_elements; elem_index++) {
        ...
        *elemdata = (*integrator)(*element, *elemdata, drin, num_particles, &param);
        ...
        element++;
        integrator++;
        elemdata++;
}

Everything being prepared before, an additional element costs a single function call: nothing!

What matters is the computation to be done, whether it's done in a single element or several does not matter. So one should not compromise the cleanliness of the code for saving one lattice element.

lcarver commented 8 months ago

So if we do not put U0 in SimpleQuantDiffPass, we have to put it as a a misalignment in the linear matrix. Which is fine. But when switching between rad on and rad off, we need to switch between having this misalignment to give U0 or to give 0.

lcarver commented 8 months ago

In fact, after some more digging. I am surprised that get_energy_loss fails for simple_ring.

If I initialise simple_ring and run get_energy_loss.

ring_full = at.load_mat('../Repo/pyce/machine_data/esrf.mat', mat_key='LOW_EMIT_RING_INJ')
U0 = ring_full.get_energy_loss()

ring_simple = simple_ring(ring_full.energy, ring_full.circumference, ring_full.harmonic_number, 0.1, 0.1, Vc, at.get_mcf(ring_full.disable_6d(copy=True)), U0=U0, TimeLag=0)

print(ring_simple.get_energy_loss(method=at.ELossMethod.TRACKING))

I get 0.

If I do exactly what is in the get_energy_lossfunction

ringtmp = ring_simple.disable_6d(at.RFCavity, at.Collective, at.QuantumDiffusion, copy=True)
print(at.lattice_pass(ringtmp, np.zeros(6)))

I get (after squeeze) [0, 0, 0, 0, -0.00042208, 0]

which is actually what I want it to return

lfarv commented 8 months ago

Output:

fullring 2531662.0976...
fastring 2531660.675...

Do I understand that a 2.10-7 relative deviation is a problem?

The QuantumDiffusion element is explicitly switched off in get_energy_loss, and the energy loss is in the T1 and T2 attributes of the M66 element. These result from the 6D closed orbit of the full ring, and that's not surprising to get this relative error in an orbit computation. It may change if you vary XYStep and DPStep.

This looks prefect to me. Or maybe I missed something?

lfarv commented 8 months ago

So if we do not put U0 in SimpleQuantDiffPass, we have to put it as a a misalignment in the linear matrix. Which is fine.

You come to the same conclusion as me, that's what fastring does ! Note that in fastring, the damping is also included in the linear element.

But when switching between rad on and rad off, we need to switch between having this misalignment to give U0 or to give 0.

Exact… This should already be a problem for fastring. Unless we move damping and energy loss in a separate element, which can be easily turned on/off.

lcarver commented 8 months ago

Sorry I edited lfarvs comment by mistake (instead of replying).

Well for fast ring, we return two lattices. One for rad on and one for rad off. So this is why we don't see this problem there. One shouldn't switch radiation on/off on the fast ring lattice. Adding the new element for damping and U0 would allow a big cleanup of fast and simple ring, but at the cost of another new PassMethod / Element. Maybe this is the best way...

lfarv commented 8 months ago

Indeed having the same lattice description for fastring and simplering would avoid solving the same problems twice!

Isolating the damping has one drawback: it adds one matrix multiplication. While both linear motion and damping are at the moment combined in a single matrix in fastring. Not tremendous, to be looked at.

So we should take the time to carefully look at all options.

lcarver commented 7 months ago

Hello @lfarv , is it acceptable to store the U0in an element as _u0, then for radon you setU0=_u0. and for radoff you set U0=0?

This could also be done for the damping times to ensure an accurate U0 computation in get_energy_loss

lcarver commented 7 months ago

Fixed in #710