-
Notifications
You must be signed in to change notification settings - Fork 37
Include Strength and IntegratedStrength in magnet elements #970
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
Conversation
|
To be noted that the notation K, H, O are not the most intuitive. Changing this would probably mean breaking backward compatibility but would allow for generalization with compact code. |
| def __init__(self, family_name: str, length: float, o: float = 0.0, **kwargs): | ||
| """ |
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.
Problem here: you change the signature of the constructor which was
Octupole(name, length, polya, poly)
which is not backward compatible. This is illustrated by the change you had to do in other files.
You could try
def __init__(self, name, length, *args, **kwargs):
if isinstance(args[0], Iterable):
polya, polyb = args[:2]
else:
polya = []
polyb = [0.0, 0.0, 0.0, args[0]]
kwargs.setdefault("PassMethod", "StrMPoleSymplectic4Pass")
super().__init__(family_name, length, poly, poly, **kwargs)to keep compatibility
Nothing prevents from adding new properties if it looks useful. I would consider the ongoing PALS project which tries to set a standard for the definition of magnetic multipoles: they chose Kn1, Kn2… for normal components and Ks1, Ks2… for skew components. And Kn1L, Kn2L… for integrated strengths. |
| @property | ||
| def Strength(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.
The Strength property is wrong for thin multipoles: it should return Infinity. This is not very useful, so the property could be moved to the Multipole class.
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.
We may argue that for thin element Strength=IntegratedStrength and it is more coherent and easier if all magnet elements have the same properties, I will keep it there and improve the docs instead
The problem is that K is now reserved for the gradient while it should be a strength... anyway we have to live with it as long as we maintain backward compatibility. To be kept in mind for future major releases I suppose |
According to the PALS definition, K is the same as Kn1. Similarly, H is Kn2/2. And so on. Adding such additional properties is easy. |
This PR proposes a minor enhancement.
The properties strength and integrated strength pointing to
PolynomB[DefaultOrder]andPolynomB[DefaultOrder]*Lengthare added. A complete octupole element with default strength propertieOis also implemented.The
Strengthproperties could eventually replaceK, H, Oto simplify the code.Few open questions remain:
DefaultOrderare not handled -> raise an error when trying to accessStrengthorIntegratedStrength, here I considered taking into account the lowest non-zero element inPolynomBbut I think it is dangerous, what do you think?DefaultOrder. Concerning this point I am considering merging the 2 passmethods, ThinMultipole would then become a special case of multipole with length=0.0, this would allow to create thin element simply by setting their length to zero.For example:
thinquadrupole = at.Quadrupole('QUAD', 0.0, 1.0)which would be a much more natural user interface.
If considered useful, this last point should be treated in a separate PR.