Skip to content

Improve Wiggler energy handling to fix #894 #960

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.

Sign up for GitHub

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

Merged
lfarv merged 8 commits into from
Aug 15, 2025
Merged

Improve Wiggler energy handling to fix #894 #960

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
ef2ca21
Add Energy to Wiggler build attributes
T-Nicholls Jul 23, 2025
a9f5819
Ensure wiggler Energy attribute is always present and consitent with …
T-Nicholls Jul 23, 2025
d12902c
Revert "Ensure wiggler Energy attribute is always present and consite…
T-Nicholls Aug 1, 2025
79fd8ae
Revert "Add Energy to Wiggler build attributes"
T-Nicholls Aug 1, 2025
c5fd65f
No longer pass through positional argument energy
T-Nicholls Aug 1, 2025
2cbcf80
Fix test warnings
T-Nicholls Aug 1, 2025
c28c514
No longer create Energy attribute for wigglers in lattice object ener…
T-Nicholls Aug 1, 2025
4bdf6ef
Update Wiggler docstring to indicate energy is ignored
T-Nicholls Aug 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions pyat/at/lattice/elements/magnet_elements.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -505,7 +505,7 @@ def __init__(
length: total length of the wiggler
wiggle_period: length must be a multiple of this
b_max: peak wiggler field [Tesla]
energy: beam energy [eV]
energy: kept for backwards compatibility but always ignored
Nstep: number of integration steps.
Nmeth: symplectic integration order: 2 or 4
Bx: harmonics for horizontal wiggler: (6, nHharm)
Expand All @@ -522,6 +522,13 @@ def __init__(
"Wiggler: length / wiggle_period is not an "
f"integer. ({length}/{wiggle_period}={n_wiggles})"
)
if energy:
warnings.warn(
AtWarning(
f"Wiggler: positional argument energy={energy} has been ignored as "
"lattice.Energy is always used instead."
)
)
super().__init__(
family_name,
length,
Expand All @@ -531,7 +538,6 @@ def __init__(
Nmeth=Nmeth,
By=By,
Bx=Bx,
Energy=energy,
**kwargs,
)

Expand Down
4 changes: 1 addition & 3 deletions pyat/at/lattice/lattice_object.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -677,9 +677,7 @@ def energy(self) -> float:
def energy(self, energy: float):
# Set the Energy attribute of radiating elements
for elem in self:
if isinstance(
elem, (elt.RFCavity, elt.Wiggler)
) or elem.PassMethod.endswith("RadPass"):
if isinstance(elem, elt.RFCavity) or elem.PassMethod.endswith("RadPass"):

@swhite2401 swhite2401 Aug 11, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use black to format the code in AT, maybe this is why this line break was there in the first place?

@T-Nicholls T-Nicholls Aug 12, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it was previously over black's 88 line length limit and was therefore split, but now we are no longer checking wigglers it is only 86 and so can go on one line

elem.Energy = energy
# Set the energy attribute of the Lattice
# Use a numpy scalar to allow division by zero
Expand Down
2 changes: 1 addition & 1 deletion pyat/test/test_basic_elements.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -552,7 +552,7 @@ def test_wiggler(rin, func):
periods = 23
bmax = 1
by = numpy.array([1, 1, 0, 1, 1, 0], dtype=numpy.float64)
c = elements.Wiggler('wiggler', period * periods, period, bmax, 3e9, By=by)
c = elements.Wiggler('wiggler', period * periods, period, bmax, By=by)
assert abs(c.Length - 1.15) < 1e-10
# Expected value from Matlab AT.
expected = numpy.array(rin, copy=True)
Expand Down
2 changes: 1 addition & 1 deletion pyat/test/test_integrators.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ def test_exact_hamiltonian_pass_with_dls_dipole(rin, func):
('GWigSymplecticPass', 'GWigSymplecticRadPass'))
def test_gwig_symplectic_pass(rin, passmethod, func):
# Parameters copied from one of the Diamond wigglers.
wiggler = elements.Wiggler('w', 1.15, 0.05, 0.8, 3e9)
wiggler = elements.Wiggler('w', 1.15, 0.05, 0.8)

@swhite2401 swhite2401 Aug 11, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't you get an error if there energy is not defined?

@lfarv lfarv Aug 11, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Wiggler energy is ignored since the Lattice energy is always used. The 3rd positional argument of the constructor is still accepted for compatibility with the very first implementation of th wiggler. In this PR, no Energy field is created in the Wiggler object, while previously, it was created and later removed by the Lattice constructor (or should have been…).

The only difference is that now a warning is issued while before the energy was silently ignored. This is the reason of the modification of the tests: avoiding this new warning. I don't like too much modifying the tests : it reveals a change of behaviour, and old lattice definitions may now suddenly throw warnings. On the other hand, this warning makes sense, silently ignoring entries is confusing. My preference is to keep this new warning, but if it looks really disturbing, it could be removed…

@swhite2401 swhite2401 Aug 12, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, now I am lost... so if I understand well energy is now always ignored and instead if the user wants to set manually the energy he should use the keyword argument Energy.

This is a rather significant change of behavior in my opinion that is not reflected in either the warning (that should say please use the keyword argument Energy to set the energy of the wiggler) or the docstring where the positional argument energy is still used.

I do not think that this is backward compatible as script using the positional argument energy will now lead to different results. While the solution I proposed:

energy= kwargs.pop('Energy', energy)

was handling all case.

For me there is now 2 options:
1-either we remove entirely the positional argument energy and user will get an error message, this is the cleaner solution but is not backward compatible (already the case)
2-we handle all case setting priorities as I proposed

I would very much prefer the first option that is simpler and cleaner but it break the rule of maintaining backward compatibility for minor releases... so I would strongly advocate for 2 until the next major release

@lfarv lfarv Aug 12, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@swhite2401

Wait, now I am lost... so if I understand well energy is now always ignored and instead if the user wants to set manually the energy he should use the keyword argument Energy.

No, an Energy keyword is ignored as well: in GWigSymplecticPass, an Energy field, if existent, is ignored. The Lattice energy is always used. And there is no change in this PR: GWigSymplecticPass is unchanged.

So there is no difference with your solution: an ignored Energy field or no Energy field is equivalent. In addition, the Lattice constructor removes the Energy field of all elements except cavities and *RadPassMethods. And even in *RadPassMethods, the Element energy is ignored.

I do not think that this is backward compatible as script using the positional argument energy will now lead to different results

No, it's compatible, there will be no change in the results: the 3rd positional argument, or any Energy keyword was already ignored. That's why I think that the new warning is useful. But you are right, the docstring should mention that this argument is kept for compatibility, but ignored.

This is slightly different in Matlab: in old lattices without RingParam element, the element energy is used. If there is a RingParam element, the behaviour is the same as in python. A .mat file created in python has always a RingParam element, so the element Energy fields are not necessary to get the correct behaviour when the file is processed in Matlab.

@swhite2401 swhite2401 Aug 12, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah sorry, I did not look all the way down to atEnergy() that in fact always returns the lattice energy...
This is all very interleaved and confusing, we should really think of cleaning this up.

However, I see in the passmethod that 'Energy' is interpreted, so it can be used in case the lattice energy=0.0. Is it possible that this happens if the lattice is defined as a list (allowed in pyAT)?
Shouldn't this be documented? Or should we just completely ignore the 'Energy' attribute and throw an Error if the lattice energy is undefined directly in the passmethod?

@lfarv lfarv Aug 12, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

atEnergy/atGamma was introduced in #816 as the standard way to access the energy in all integrators, with the following constraints:

  • backward compatibility in Matlab for old lattices without RingParam,
  • Matlab and python compatibility for new lattices,
  • work for energy ramping

all that in a single line, identical in all integrators except RFCavityPass. For a detailed explanation, see #816.

However, I see in the passmethod that 'Energy' is interpreted, so it can be used in case the lattice energy=0.0

Energy is always interpreted in all integrators, because of Matlab old lattices. But in python, I do not see any particular case for "lattice energy=0.0". 0.0 is used for energy or gamma (except for RFCavityPass again):

#define atEnergy(ringenergy,elemenergy) (ringenergy)

If an error should be thrown for energy==0.0,, it could be added in this python definition of atEnergy.

For lattices as "lists", the energy can be provided with an energy keyword in lattice_track (documented).

@swhite2401 swhite2401 Aug 13, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I still think this is really not clean and for someone not familiar with AT it is basically impossible to understand the behavior... I guess this is the kind of things we have to live with to maintain backward/matlab compatibility so fine for the moment.

wiggler.PassMethod = passmethod
func(wiggler, rin)

Expand Down