Improve Wiggler energy handling to fix #894

[T-Nicholls]

• Energy has been added to the _BUILD_ATTRIBUTES of Wiggler, so it will now be automatically popped and passed as the positional argument energy when loading from lattice files
• energy is still an optional argument when creating a Wiggler
• Wigglers will now always have an Energy attribute, and it will be made the same as the Lattice's energy
• The precedence order when determining the lattice energy in params_filter is unchanged
• The Energy attribute on elements (including wigglers) remains unused in Python, as the Lattice's energy is always used

[swhite2401]

@T-Nicholls , I went through the discussion in #894, and I am not so sure what you propose is consistent with what @lfarv had suggested. My understanding is that you shall handle upper case and lower energy arguments in the constructor to avoid any issues. In this case you would only need to add the following line to the wiggler constructor I believe:

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

and then your super().init() would not complain anymore about having twice Energy...or am I missing something?

Furthermore, looking at the passmethod, there are again there several level of prioritization:
1- when reading the attribute
2- in atGamma()

This is a bit messy in my opinion with to many layers. I would tend to think that we could add gamma to the parameters structure and get rid of atGamma() and atEnergy() using only 1 as the prioritization method. but this is maybe to much for this PR.

[lfarv]

Hello @T-Nicholls and @swhite2401.

I fully agree with @swhite2401: the single line added to the wiggler constructor is the simplest solution and solves the problem. Such a syntax is used in other constructors. It achieves the usual priorities where the keyword argument has priority over the positional argument.

Concerning the atEnergy/atGamma duo, this was set up to unify the handling of energy ramping between Matlab and python:

• Matlab uses the priority lattice_energy > element_energy (lattice_energy may be missing). Python only uses lattice_energy,
• some passmethods need energy, others need gamma.

Changing this would need a review of all the integrators, it's another story.

[swhite2401]

Concerning the atEnergy/atGamma duo, this was set up to unify the handling of energy ramping between Matlab and python

Ok I did not follow this development, I just came across these functions and was wondering if they were really needed.

[T-Nicholls]

Hello @lfarv & @swhite2401 ,

I chose this way because I didn't want to change the behaviour of the Wiggler element constructor, just return it to the same as before the bug was introduced:

• Before Fix wiggler radiation #822, Energy was in the _BUILD_ATTRIBUTES so it was popped and passed as the energy argument when creating elements from files and we didn't get the error that we do now, but it never supported being passed to the constructor in kwargs
• After Fix wiggler radiation #822, passing Energy always gives an error and isn't supported at all

In summary, we've never supported both energy and Energy being passed with one taking priority over the other, and so I was hesitant to add that new functionality since it wasn't absolutely required to fix the bug.

If you think it is worthwhile to add this new functionality, then I would be happy to add the line you propose so that both are supported and Energy takes priority.

[lfarv]

Hello @T-Nicholls

I see problems in your implementation: by modifying the _BUILD_ATTRIBUTE of the Wiggler, you induce detrimental consequences:

• you change the repr representation of the Wiggler by introducing a useless 5th positional argument. With possible side effects in file saving (.m files),
• but, more important, you re-introduce a mandatory Energy attribute in the Wiggler. Since Fix wiggler radiation #822 it is ignored because the energy is always taken from the Lattice object. Keeping this attribute is harmless but undesirable because it may be confusing if its value differs from the lattice one. Note that the 5th positional argument is kept for compatibility with old python code using it, but is now ignored,
• it forces a modification of lattice_object.py. Apart from possible side effects, this introduces a special case for Wiggler elements, which should be avoided.

So in the end, I would not modify _BUILD_ATTRIBUTES. @swhite2401's solution solves the .mat problem without changing anything in the behaviour, neither constructor syntax nor generated Wiggler element. But there is an even simpler solution: in the call to the Wiggler parent's constructor, remove the

            ...,
            Energy=energy,
            ...,

line. Instead of creating an Energy attribute which will be deleted later, don't create it, unless it's set in kwargs.

• In an explicit call to the Wiggler constructor, no Energy attribute is created, this is the intended behaviour,
• When reading a .mat file, an Energy field in the file, if existing, will be set in kwargs, will create a Wiggler attribute which will participate in deducing the lattice energy, as now, and will be later deleted. This is again the intended behaviour.

NOTE: this reveals another small bug: in the energy setter of the Lattice property, an Energy attribute is created in RFCavities and Wigglers. This should be limited to RFCavities. It was probably forgotten in #822. No consequence when running PyAT, but it should be corrected in another PR.

[T-Nicholls]

Hello @lfarv

I am fine with the solution you propose, so I have implemented it here and I've also fixed the lattice Energy setter

[swhite2401]

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

[T-Nicholls]

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

[swhite2401]

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

[lfarv]

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]

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]

@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]

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]

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]

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.

[lfarv]

@T-Nicholls:

In the docstring of Wiggler.__init__, could you change the description of the energy argument:

     energy:      kept for compatibility but ignored