atcollab / at

Accelerator Toolbox
Apache License 2.0
48 stars 31 forks source link

wrong tune in atplot when lattice contains an IdTablePass (matlab) #561

Closed oscarxblanco closed 1 year ago

oscarxblanco commented 1 year ago

Dear all,

atplot shows a wrong value for the ring tune when an element with pass method IdTablePass is inserted in the ring.

It seems to come from lines 42 to 43 in xplot.m where the ring is splitted into smaller elements.

r2=cellfun(@splitelem,ring(el1:el2-1),'UniformOutput',false);
splitring=cat(1,ring(1:el1-1),r2{:},ring(el2:elt0));

https://github.com/atcollab/at/blob/master/atmat/atplot/xplot.m

Unfortunately, this method is incompatible with IdTablePass because the inner tables already contain the integrated kick given by a magnetic field all along the element length. Therefore, the split will change the nominal length of the element but the kick will be artificially increase by as many elements as it is splitted.

One way to correct this would be to include an longitudinal scale factor, or alternatively, avoid splitting elements with pass method IdTablePass.

This issue might also impact the python development in issue #558 , where a similar solution would be needed.

Best regards, o

Here is the result of a short example, where the ring parameters from atlinopt6 and atplot show different vertical tune. The difference comes from an IdTablePass element that has been splitted in three to make the plot. image

oscarxblanco commented 1 year ago

One possible quick solution is to include a pass method check inside splitelem in line 77 of xplot.m : https://github.com/atcollab/at/blob/master/atmat/atplot/xplot.m

        if isfield(elem,'Length') && elem.Length > 0 ...
                && ~strcmp(elem.PassMethod, 'IdTablePass')

Otherwise, one could remove the indexes with IdTablePass method from the splitelem call, and then, reconstruct the ring in intervals.

lfarv commented 1 year ago

@oscarxblanco : That's a good solution. Do you open a PR for that or shall I do it ?

In PyAT, this can be solved by overloading the divide() method of the InsertionDeviceKickMap element: it should return [self], as the base Element, meaning that the element is not divided

oscarxblanco commented 1 year ago

@lfarv I could do it. What would be the name of the branch ?

oscarxblanco commented 1 year ago

@lfarv In the case of the InsertionDeviceKickMap in python I checked the source of the parent class Element https://atcollab.github.io/at/_modules/at/lattice/elements.html#Element and I see that [self] is already implemented in the divide() method.

The documentation of the class Element, however, needs to be updated, as it refers to a drift which inherits from another parent called LongElement.

lfarv commented 1 year ago

Ah, InsertionDeviceKickMap inherits directly from Element ! As it has a length, I expected it to inherit from LongElement… However the LongElement methods (divide and merge) look useless for you, so it can stay like that, and there is nothing more to do: it cannot be divided.

The documentation of the class Element, however, needs to be updated, as it refers to a drift which inherits from another parent called LongElement.

I don't see what needs to be modified… Can you tell more ?

oscarxblanco commented 1 year ago

@lfarv The doc string inside the divide method in the Element class. It says that Element can be divided, but, the code just returns self.

lfarv commented 1 year ago

This defines what the divide method does: split the element into parts. And it applies to all its descendants unless a new doc string is provided (look at LongELement in the html doc: the doc of the Element version is repeated). The practical implementation depends on the kind of element, but the goal is always the same. So I would keep as it is…

oscarxblanco commented 1 year ago

ok, thank you again @lfarv

lfarv commented 1 year ago

Fixed in #563.