Implement AC-Dipole Elements #660
Conversation
… and ensure tune values are within [0, 1) range
…ion, and update assertions for accuracy
- Consolidated ACDipole classes into a single ACDipole class that handles both tracking and twiss modes. - Updated the constructor to accept parameters for voltage, frequency, lag, ramp, plane, twiss_mode, beta_at_acdipole, and natural_q. - Removed the old ACDipoleThickHorizontal, ACDipoleThickVertical, ACDipoleThinHorizontal, and ACDipoleThinVertical classes. - Modified the tracking logic to apply kicks based on the specified plane (horizontal or vertical). - Updated tests to reflect the new ACDipole structure, removing orientation parameters and replacing them with plane parameters. - Added a new test file for ACDipole twiss behavior. - Removed the old thin AC dipole tests as they are no longer necessary. - Updated C source files to reflect the new ACDipole structure and removed unnecessary files.
szymonlopaciuk
left a comment
szymonlopaciuk
left a comment
There was a problem hiding this comment.
It's good, just a few really minor comments.
| if not ( | ||
| (isinstance(ramp, (list, tuple)) or hasattr(ramp, "__iter__")) | ||
| and len(ramp) == 4 | ||
| and all(isinstance(int(x), int) for x in ramp) | ||
| ): | ||
| raise ValueError( | ||
| "The ramp parameter must be a list of four integers: [ramp1, ramp2, ramp3, ramp4]." | ||
| ) |
There was a problem hiding this comment.
I find this a bit complicated for what it does.
isinstance(ramp, (list, tuple))implieshasattr(ramp, "__iter__"), so the first check is not necessary- In most cases
__iter__implies a__len__. Sure there are exceptions, but on the other hand it's not strict enough anyway:setpasses all the checks, but it's probably not desired. You'd probably also need to add a check to assert that the list is sorted as well? - Is the check for integer not a bit strict? What if someone gives a float for whatever reason?
I agree the built-in error message in Xobjects is not the clearest, so having some sort of check is an improvement for the user. I'm not sure what is the best way, but I'd try to simplify. One possibility:
if len(ramp) != 4 or list(ramp) != sorted(ramp):
raise ValueError(
"The ramp parameter must be an increasing list of four integers."
) There was a problem hiding this comment.
How about:
if not (len(ramp) == 4 and all(v == int(v) and v >= 0 for v in ramp) and ramp == sorted(ramp)):
raise ValueError(
"The ramp parameter must be an increasing list of four integers:"
"[ramp_up_start_turn, ramp_up_end_turn, ramp_down_start_turn, ramp_down_end_turn]."
)Also, I see no problem with using a set, even though this would fail it now.
There was a problem hiding this comment.
ramp == sorted(ramp) will sadly fail for (1, 2, 3) because of the type. Could go with np.all(np.diff(ramp) >= 0) if you prefer, though I don't find that better.
Also afk at the moment and can't check, but is v == int(v) a reliable check? Maybe I prefer isinstance after all, less annoying if things go wrong.
Sorry for splitting hairs.
szymonlopaciuk
left a comment
szymonlopaciuk
left a comment
There was a problem hiding this comment.
See the one comment
2 participants
Description
Here I implement a thick and a thin AC-dipole along with tests.
Will the documentation inside the element classes be automatically inserted? I.e. Do I need to write additional documentation?
Also, help with how to run the GPU contexts would be nice!
Checklist
Mandatory:
Optional: