search

atcollab / at

Accelerator Toolbox
Apache License 2.0
48 stars 31 forks source link

Update momentum aperture matlab #786

Closed oscarxblanco closed 1 week ago

oscarxblanco commented 2 weeks ago

This is a Work In Progress pull request to update the arguments in the momentum aperture calculation on matlab. The idea is to have the possibility to pass the closed orbit, but, the default behaviour is preserved, i.e. ignoring the closed orbit. The reason not to change the behaviour is to avoid a change on the results from this commonly used function.

I mentioned this update in the discussion #753 . It might be already solved many times, but, it seems worth to provide a common solution.

oscarxblanco commented 2 weeks ago

Dear @swhite2401 , this is a modification of the matlab MomAperture_allRing and momentum_aperture_at functions in order to pass the closed orbit. Additionally, I put keyword options that were previously hard-coded, and verbose to two levels. As this is a well known function I decided to keep it backwards compatible and by default it ignores the closed orbit.

If you agree to accept this change, who could review it ? Here is the matlab code that could be used for tests :

corbit = findorbit(THERING,positions);
tic
[dpN,dpP]=MomAperture_allRing(THERING,positions,Nturns, ...
    'reforbit',corbit, ...
    'verbose',2);
toc
swhite2401 commented 1 week ago

@oscarxblanco , for matlab I would prefer that @lfarv or @simoneliuzzo reviews the code

simoneliuzzo commented 1 week ago

@oscarxblanco ,

this looks very nice!

I do not use this function since a very long time, as I run the computations in the cluster only.

I do not see anything wrong at first sight in your code, it seems only better, documented and backward compatible.

Using the matlab argument parsing functions I think is also a very good choice.

should I test or you did already?

best regards Simone

oscarxblanco commented 1 week ago

Dear @simoneliuzzo , I tested it to compare results with another function we were writing at https://github.com/atcollab/at/pull/773 . Indeed, this is not a big change, I wanted to be able to pass the closed orbit and see the verbose. Apart from that, several parameters that were hard coded are now available to the user.

I would suggest you to use a lattice with some closed orbit and compare few or more points of the momentum aperture. The result might be slightly biased because of islands of instability that depend on the step, as you can see here below image

oscarxblanco commented 1 week ago

@lfarv , yes, I tested it. I merge !