atcollab / at

Accelerator Toolbox
Apache License 2.0
48 stars 31 forks source link

Fix Lattice.reduce #733

Closed lfarv closed 6 months ago

lfarv commented 8 months ago

Fixes #732: @simoneliuzzo reported a different behaviour the "reduce" function between Matlab and python: in python, drifts with different RApertures or EApertures attributes are considered compatible and are merged by Lattice.reduce()

The resulting lattice behaves differently from the original one, so this is considered as a bug.

This PR solves this.

Side remark: the new pytest release (8.0.0) crashes in the AT sequence test. This looks related to a problem with pytest_lazy_fixture. We have to force "pytest < 8.0.0" temporarily until this is understood.

simoneliuzzo commented 7 months ago

Dear @lfarv,

I tested for several lattices. Only for one of them the reduced lattices have the same number of elements in matlab and python AT after reduce.

The issue seems now an RF cavity with pass method DriftPass.

It is reduced in matlab, while it is not reduced in python.

I think not removing the RF cavity, even if it is DriftPass is the best solution.

matlab:

function ring=comp_mat_1776() ring={... atquadrupole('QDL3A',1,-0.01368024032,'NumIntSteps',60);... atdrift('DR_375',74.9999999999992,'NumIntSteps',60);... atquadrupole('QFL3A',1,0.01348891689,'NumIntSteps',60);... atdrift('DR_75',75,'NumIntSteps',60);... atquadrupole('QDL2A',1,-0.01539394697,'NumIntSteps',60);... }; end

python:

function ring=comp_pyt_1776() ring={... atquadrupole('QDL3A',1,-0.01368024032,'NumIntSteps',60);... atdrift('DR_375',37.5,'NumIntSteps',60);... atrfcavity('RFC',1e-12,0,0,135000,182500000000,'DriftPass');... atdrift('DR_375',37.4999999999982,'NumIntSteps',60);... atquadrupole('QFL3A',1,0.01348891689,'NumIntSteps',60);... }; end

simoneliuzzo commented 7 months ago

Dear @lfarv,

when setting atenable_6d and r.enable6d() there are only two differences left for my test: the ringparam element: it is there in pyAT but not in matlabAT, so there is one extra element in the reduced python lattice a drift with 0 length that is reduced by python and not reduced by matlab . in this case I think python does the correct choice.

matlab:

function ring=comp_mat_rad_end() ring={... atdrift('DR_015',0.149999999994179,'NumIntSteps',60);... atquadrupole('QD0AL',1.75,-0.1654715921,'StrMPoleSymplectic4RadPass','NumIntSteps',60,'Energy',182500000000);... atdrift('DR_22',2.19999999999709,'NumIntSteps',60);... atdrift('DR_0',0,'NumIntSteps',60);... }; end

python:

function ring=comp_pyt_rad_end() ring={... atquadrupole('QD0BL',1.75,-0.130722557759,'StrMPoleSymplectic4RadPass','NumIntSteps',60,'Energy',182500000000);... atdrift('DR_015',0.149999999994179,'NumIntSteps',60);... atquadrupole('QD0AL',1.75,-0.1654715921,'StrMPoleSymplectic4RadPass','NumIntSteps',60,'Energy',182500000000);... atdrift('DR_22',2.19999999999709,'NumIntSteps',60);... }; end

lfarv commented 7 months ago

Dear @simoneliuzzo, Thanks for testing! concerning your 1st comment:

I agree with you on the fact that RF cavities should be kept. In this respect, the python version in this PR looks correct. Modifying the Matlab version is possible, but would modify the behaviour. It can be done if we consider the present behaviour as a bug. What do you recommend?

Side remark: what is the motivation for a "long" cavity element, knowing that a long cavity is treated as a thin cavity surrounded by two drifts? Using 1.0e-12m long elements is a numerical non-sense! But note that in this case, an inactive thin cavity with IdentityPass would be simply eliminated in atreduce: the problem is the same.

lfarv commented 7 months ago

@simoneliuzzo: About the 2nd comment:

I do not clearly see how your example is related to the RingParam element: this element does not exist in python, it's ignored when reading a .mat file (unless using keep_all=True), and introduced again when writing a .mat file.

What I see in your Matlab output is a Drift with length 0 and DriftPass, not a RingParam element. Where does this Drift come from, and why isn't it reduced? Is it the last element of the lattice? A partial view of the original lattice in this region would help.

Finally, in both cases, the python version in the PR looks correct to me. So:

  1. Do you consider this python version correct? Can it be merged?
  2. Do you think the Matlab version should be modified, to preserve the RF cavities and to eliminate possibly remaining 0-length drifts? This can be done in another pull request, or in this one since it's related. What is your advice?
lfarv commented 6 months ago

Dear @simoneliuzzo,

This PR involves a critical module in PyAT (elements.py) also involved in other PRs. That's why I would like to go on. Considering you previous comments, here is my proposal:

  1. I think that this PR fully fixes the issue you originally posted: RApertures and EApertures are ignored in Lattice.reduce. If you agree (and approve), we merge the PR as it is, which allows me to go on,
  2. All your other comments deal with the Matlab version. If you think it should be modified, to preserve the RF cavities and to eliminate possibly remaining 0-length drifts, we open a new issue for that, to be solved in a different PR

My fear is that if we try to fix many different things simultaneously, it may take many iterations…