bugfix in rotation angles and improved help of rotate_elem

[swhite2401]

This PR answer #709 and #712. The rotate_elem function was poorly documented with wrong information and featured a mix of small angle approximation and exact calculation.
These issues are resolved in the updated function.

[swhite2401]

@oscarxblanco I did very quick check and it seems to be fine now, please let me know if this what you were expecting, thanks!

[oscarxblanco]

Hi @swhite2401,
you moved problem to other components by redefining the sense of rotation in the help message.

In your new definition (counter-clock wise) positive pitch gives a positive vertical angle, but then, as you apply the rotation in the middle of the element you should have position components in T1 and T2 switching sign too.

ring[0].T1=array([ 0.        ,  0.        , **-0.0298004**wrongsign** , -0.20271004,  0.        ,
        0.        ])
ring[0].T2=array([ 0.        ,  0.        , **-0.0298004**wrongsign** ,  0.20271004,  0.        ,
        0.        ])

You could either go back to clock-wise definition and fix the bug in the angles, or, keep your new definition and fix the bug in the position components. I would suggest to fix the angle sign bug because pitch typically is defined to point downwards.

Wrt to the help message

    ... The change
    of effective length of the element seen by the particle is not accounted
    for: it is over estimated in this approximation. This effect is however
    negligible for small angles.

sounds misleading because the effective length could have a different meaning considering border effects and so on. I would suggest the following, or something similar

    ... Following the small angle approximation longitudinal offsets are set to zero.

The help message still misses the information about the rotation pivot. I would suggest the following, or something similar

    The transformations are not all commutative, the pitch and yaw
    are applied first and the tilt is always the last transformation
    applied. The element is rotated around its midpoint.

[swhite2401]

Thanks @oscarxblanco and sorry about the sign confusion... I have implemented the requested changes

[swhite2401]

I have also fixed some unrelated PEP8 non conformity throughout the file

[oscarxblanco]

Hi @swhite2401 ,
clock-wise pitch and yaw have different signs. Positive pitch points downwards and goes with a negative sign, while, positive yaw points outwards and goes with a positive sign. In your implementation you assume pitch and yaw have the same sign which is not the case.
Also, I see that you have a mix of $\sin$ and $\tan$ functions that do not correspond to the effect of rotations. Of course, the error is somehow small because $\sin(x)\approx\tan(x)\approx x; \forall x\ll 1$, but, it could mislead any further development. If you prefer to leave just $x$ with the correct signs, and document the small angle approximation, I'm ok with that.

[swhite2401]

Ok , sorry I think I am going too fast on this because I am doing too many things at a time. Let me take a step back a redo the math, I will propose a consistent solution.

To summarize the situation:
1-yaw has the wrong sign, this is easily corrected
2-the T vectors are correct providing this sign error is fixed
3-The tilt calculation is correct
4-The application of the yaw and pitch on the total rotation matrix are wrong (lines 910->915)

[swhite2401]

So looking back at this in details and what was done in the past it seems only the sign of the yaw was inconsistent with the convention defined in the help. The terms relating to pitch and yaw in the R matrices are there just to cancel the energy dependency on the rotation angle that we would get by using T1, T2 only.

Vector T:
1- positive yaw, element pointing outwards, particle coordinates changes: a positive, psi negative
2- positive pitch, element pointing downwards, particle coordinates changes: a negative, psi positive

T1 = [a, -psi, -a psi, 0, 0]
T2 = [a, psi, -a -psi, 0, 0]

However 2nd and 4th coordinates are, not angles but momenta. The angle is x'=px/(1+dp) . To compensate for this we use the the R matrix such that 2nd and 4th coordinates are incremented by (1+delta)*psi. See discussion in #512 and #519 for this.

@oscarxblanco do you agree with this? Or is there something else I am missing?

[swhite2401]

Maybe I should be documenting this in the help so that we don't have to go through it again in the future

[oscarxblanco]

Hi @swhite2401,
signs are now ok. The trigonometric functions are wrong. Somehow you swapped $sin$ and $tan$ in the expression. I have described the problem in more detail on the code.

[swhite2401]

Hi @swhite2401, signs are now ok. The trigonometric functions are wrong. Somehow you swapped sin and tan in the expression. I have described the problem in more detail on the code.

I don't see you suggestions you have to submit them I think

[swhite2401]

All suggested modifications applied, thanks!

[oscarxblanco]

Dear @swhite2401 ,
I checked $T1$ and $T2$ for pitch, yaw and tilt angles and they agree with what I expect and obtain from SCgetTransformation.m within the limits of the small angle approximation.
However, it took me a little bit more to check the rest of the calculation because I disagreed with $R1$ and $R2$ matrices. The rotation matrices should have an impact on the longitudinal coordinate even to first order. This is visible in SCgetTransformation, here below an example (the same example with a drift of 0.3 m and 0.2 rad pitch).

R1 =
    1.0000   -0.0031         0         0         0         0
         0    1.0000         0         0         0         0
         0         0    1.0203   -0.0031         0         0
         0         0         0    0.9801    0.1987         0
         0         0         0         0    1.0000         0
         0         0    **0.2027**   -0.0006         0    1.0000
R2 =
    1.0000    0.0031         0         0         0         0
         0    1.0000         0         0         0         0
         0         0    1.0203    0.0031         0         0
         0         0         0    0.9801   -0.1987         0
         0         0         0         0    1.0000         0
         0         0  ** -0.2027**   -0.0006         0    1.0000

But it is set to zero by rotate_elem

ring[0].R1=array([[1.   , 0.   , 0.   , 0.   , 0.   , 0.   ],
       [0.   , 1.   , 0.   , 0.   , 0.   , 0.   ],
       [0.   , 0.   , 1.   , 0.   , 0.   , 0.   ],
       [0.   , 0.   , 0.   , 1.   , 0.199, 0.   ],
       [0.   , 0.   , 0.   , 0.   , 1.   , 0.   ],
       [0.   , 0.   , **0**.   , 0.   , 0.   , 1.   ]])
ring[0].R2=array([[ 1.   ,  0.   ,  0.   ,  0.   ,  0.   ,  0.   ],
       [ 0.   ,  1.   ,  0.   ,  0.   ,  0.   ,  0.   ],
       [ 0.   ,  0.   ,  1.   ,  0.   ,  0.   ,  0.   ],
       [ 0.   ,  0.   ,  0.   ,  1.   , -0.199,  0.   ],
       [ 0.   ,  0.   ,  0.   ,  0.   ,  1.   ,  0.   ],
       [ 0.   ,  0.   , **0.**   ,  0.   ,  0.   ,  1.   ]])

It seems you are setting to zero any longitudinal effect even to first order. This could be a problem depending on the machine, but, as many elements won't change the longitudinal coordinate and the rotations $R1$ and $R2$ act to cancel one another at the end I expect a relatively small error in the longitudinal coordinates.

It might however be a concern if rotations and harmonic cavities are combined.

Hopefully the help message warning about it would be enough for the moment,

    ... Following
    the small angles approximation the longitudinal shift of the particle
    coordinates is neglected and the element length is unchanged.

I will approve this merge whenever the branch is updated.

[swhite2401]

@oscarxblanco thanks for looking through this, yes indeed longitudinal coordinate changes are ignored because they have to come together with a reduction of the element length and the insertion of drift spaces which seems rather complicated for something that would have very small impact for most usage cases.

Do you request any further modifications? Your last comment seems to indicate so but I don't really get what it should be as long as we accept that the longitudinal shift is neglected, do you want me to improve the help?

[oscarxblanco]

Dear @swhite2401 , I have no more requests.
I agree with you in that this should be ok in most of practical cases.