PyAT: improved timelag solver

[swhite2401]

In get_timelag_from_U0 the numerical synchronous phase solver was failing in some cases in the presence of multiple rf systems, such as higher order cavities.
This PR proposes a simpler more robust version.

[lcarver]

I tested 2 cases. Single and multi harmonic. For both cases when the returned timelag is set on the cavities using

at.set_value_refpts(ring, cavpts, 'TimeLag', tl, copy=False)

then the equilibrium position for ct was on the order of 1e-5.

On closer inspection, I realised that I am not able to trigger the warning seen on line 163 of energy_loss.py. I put a 4th harmonic cavity with very high voltage (6.5e6/1.5) which gives multiple possible phis but it doesn't seem to catch the warning.

[swhite2401]

On closer inspection, I realised that I am not able to trigger the warning seen on line 163 of energy_loss.py. I put a 4th harmonic cavity with very high voltage (6.5e6/1.5) which gives multiple possible phis but it doesn't seem to catch the warning.

Could you send a figure of the wave form you got for this one? the initial points for the 2 solutions are at +/-lambda/4 around TimeLag so it is possible that if a solution is far off these initial values it is not found

[lcarver]

it finds a solution, but it doesn't find the others so doesn't throw a warning

[lcarver]

It's ok now. If I keep the harmonic TimeLag=0 then it triggers the warning. For TimeLag=0.1 (what you see in the figure) then it doesn't. So the warning can be triggered. but I guess the question now is why it doesn't find the second solution on the left of the main one?

[swhite2401]

It's ok now. If I keep the harmonic TimeLag=0 then it triggers the warning. For TimeLag=0.1 (what you see in the figure) then it doesn't. So the warning can be triggered. but I guess the question now is why it doesn't find the second solution on the left of the main one?

Right, can you send me your lattice?

[swhite2401]

It's ok now. If I keep the harmonic TimeLag=0 then it triggers the warning. For TimeLag=0.1 (what you see in the figure) then it doesn't. So the warning can be triggered. but I guess the question now is why it doesn't find the second solution on the left of the main one?

Right, can you send me your lattice?

There was a logic issues settings the boundaries and initial guesses, this is fixed. In addition the range is now divided in 8 initial guesses over the full wake length on the main RF, instead of just 2 before. This should mitigate the issue with solutions not found

[lfarv]

Please put the modifications of lattice_object.py and elements.py in a different pull request. They don't affect your initial PR (unless there is a diffusion element in the lattice). It avoids mixing unrelated things, makes validation easier and makes history easier to read (think of listing the changes in the release notes of the next version)… Thanks!

[swhite2401]

As request I have created #453 to propose the QuantDiffElement and the modified radiation_on/off()

[lfarv]

For the computation, I trust @lcarver and you. But:

• Why changing the default method for TRACKING ? It's decoupled from the topic of this PR: get a better solution for the equation V(cτ)= U₀, whatever U₀. INTEGRAL is robust, fast, works with or without radiation. Even if it may be less accurate, it's probably good enough in most cases as an initial guess for find_orbit. Why not keeping TRACKING for the cases where INTEGRAL is not accurate enough?

  UPDATE: find_orbit forces the default to TRACKING. So my remark on orbit search is irrelevant. But remains: why changing something without justification?

• Why changing math.pi to numpy.pi ? I have no opinion on which is best, apart from matching its type (float of numpy scalar array) to the type of the rest of the expression… Here it's mixed.

[swhite2401]

• UPDATE: find_orbit forces the default to TRACKING. So my remark on orbit search is irrelevant. But remains: why changing something without justification?

Exactly for that reason: we have to be consistent in the default we use and stick to the decision made in other places.
TRACKING is more precise and will give the correct result in any condition. I questioned the choice of TRACKING for find_orbit once because I thought integral was more robust but you replied exactly this to me.

[swhite2401]

• Why changing math.pi to numpy.pi ? I have no opinion on which is best, apart from matching its type (float of numpy scalar array) to the type of the rest of the expression… Here it's mixed.

I have no opinion on that. I always use numpy rather than math, but this is just an habit. Isn't the expression mixed anyway because of numpy.arcsin(). Again we have to be consistent an use the same everywhere...shall we add the preferred one to constants.py?

[lfarv]

I have no opinion on that. I always use numpy rather than math, but this is just an habit. Isn't the expression mixed anyway because of numpy.arcsin(). Again we have to be consistent an use the same everywhere...shall we add the preferred one to constants.py?

So no reason to change anything, let's keep the expression type decide, and in this particular case it's mixed. I was just curious about the reason for the change !

So it's ok for me.

[lfarv]

ok

[swhite2401]

Ok I merge then