atcollab / at

Accelerator Toolbox
Apache License 2.0
48 stars 31 forks source link

Bug Fix & Update: Wiggler diffusion matrix #759

Open joanarenillas opened 2 months ago

joanarenillas commented 2 months ago

Dear all,

This is an ongoing Work In Progress, which aims to correct some bugs found in GWigSymplecticPass.c and GWigSymplecticRadPass.c, as well as updating gwigR.c and ohmienvelope to include the computation of wiggler diffusion matrices. To do so, a new function (FDW.c) is currently being developed, following the work of @mashl.

More on current and past issues on wiggler passmethods can be found in #715 and #749.

No need to review yet.

joanarenillas commented 2 months ago

Dear @swhite2401,

I am currently cleaning up the code in FDW.c, which implements the computation of diffusion matrices for wigglers. I am having a little bit of trouble understanding how to code the Matlab mex function and the corresponding Python version. My guess is that it should look similar to the code in findmpoleraddiffmatrix.c, but I am not entirely sure whether I can just use the same lines of code or a more detailed makeover is necessary.

Could I get some advice with this?

Regards,

Joan

lfarv commented 2 months ago

Hello @joanarenillas,

Sorry for my late intervention, but I was away until now and just had a look to your proposal.

There is already a git branch dedicated to the computation of diffusion matrices, called diffusion_matrices, with a corresponding pull request: #742. In its discussion, you will find the motivation and the proposed solution.

In short, the idea is to have the computation of diffusion matrices as modular as the tracking itself, so that introducing a new kind of element, including its contribution to equilibrium emittances, can be made by simply dropping a new C file in the atintegrators directory,

Since the computation of diffusion matrices involves the tracking of the closed orbit through the element, a simple way is to add the diffusion as an optional computation in the integrator C file itself.

For backward compatibility, we cannot change the signature of the integrator, so to indicate the need for the computation of the diffusion matrix, we added a new variable in the struct parameters structure, called bdiff. bdiff is a (double *) pointer. If bdiff == NULL, it means that no computation of diffusion matrix is needed (simple tracking). Otherwise, bdiff points to an array of 36 doubles initialised with zeros. The integrator is expected to fill this array with the diffusion matrix of the element.

Existing integrators ignore this new variable, so are automatically considered as not contributing to diffusion. In the diffusion_matrices branch, most of the *RadPass have been modified to include the computation of diffusion matrices (still missing the so-called “Exact” integrators).

I think you should go directly to this new scheme by basing your working branch on diffusion_matrices instead of master. Then your modifications will concern only a single file: GWigSymplecticRadPass.c.

For an example, look at StrMPoleSymplectic4RadPass,c in the diffusion_matrices branch: it's drift-kick sequence in cartesian geometry, like the wiggler.

From what I can see, your new FDW.c file is a good starting point for this upgraded GWigSymplecticRadPass.c. I does both tracking and computation of the diffusion matrix. You just need to make the computation of diffusion optional by enclosing it in something like:

if (bdiff) {
    wigglerM(...);
    wigglerB(...);
    ATsandwichmmt(...);
    ATaddmm(...);
}

and to add a loop on particles for the "normal" tracking (of course, the computation of diffusion matrices is requested only with a single particle: the closed orbit).

ZeusMarti commented 2 months ago

Hi @joanarenillas, @lfarv and @swhite2401,

Besides Joan contributing to the diffusion_matrices branch, which I think it is a good idea, the present MR solves the #749 bug which I think is already a good reason to merge it, after your supervision of course.

Best,

lfarv commented 2 months ago

@ZeusMarti, @joanarenillas:

Besides Joan contributing to the diffusion_matrices branch, which I think it is a good idea, the present MR solves the https://github.com/atcollab/at/issues/749 bug which I think is already a good reason to merge it, after your supervision of course.

Ok, no objection! I have a few comments:

Otherwise, for me we could merge this branch.

Also, one should think soon of converting this to the new architecture for computation of diffusion matrices: #742. This branch was designed to integrate easily wigglers but also any future kind of element. Though apparently it did raise much interest since it was opened…

joanarenillas commented 1 month ago

Dear @lfarv,

Regarding your comments:

After this branch is merged, we are planning on adapting the computation of wiggler diffusion matrices of this branch to #742, so that wigglers are easily integrated.

Regards,

Joan

lfarv commented 1 month ago

I have no more comment. I am not able to appreciate the correctness of the result, I'll trust you . If @ZeusMarti and you are confident that everything is now correct, I can approve and merge, but if you have further validation to do, let me know.

joanarenillas commented 1 month ago

Dear @lfarv,

We are currently working on a benchmark to test our results. We hope to complete it soon. Once we have it, we will let you know the results so you can approve and merge.

Regards.

joanarenillas commented 1 month ago

Dear @lfarv,

We are having difficulties in completing the benchmark for the new set of integrators, since the particle dynamics are more complex than we anticipated. We have tried solving the Lorentz equation including energy loss (gamma factor is not constant) and taking into account the Abraham-Lorentz force. Nevertheless, the results we obtain are not 100% consistent.

Regarding the references on the Hessian matrices, we have not managed to get any from @mashl, who initially did the computation. I have however reviewed and summarised his calculation in a report which I am happy to share with you if its is of your interest. I have also included some documentation on the wiggler passmethods as you suggested.

Although the benchmark is not yet finished, I think it is still worth merging this branch, since it fixes many reported bugs with these passmethods, and includes the diffusion matrix computation for the wiggler. Moreover, the tracking in the new passmethods do not introduce significant deviations from the current version (they are smaller than 10^(-8) m).

My internship period at ALBA will finish soon and @ZeusMarti will take over from now.

Kind regards,

Joan

swhite2401 commented 1 month ago

I agree with the merge, this is a major improvement. However, before that it would be nice to add simple unit tests. Is your report publicly available on the web? If not adding it, or a summary of it to the documentation pages would be very nice

joanarenillas commented 1 month ago

Hello @swhite2401,

The report will be available online in the following weeks after it is reviewed by the ALBA accelerator's team.

ZeusMarti commented 3 weeks ago

Hi @swhite2401, by "unit tests" you mean adding an additional test definition in at/pyat/test? What kind of check do you have in mind? a tracking result through the element with and without radiation could be enough?