-
Notifications
You must be signed in to change notification settings - Fork 37
Fix bug wiggler rounding #1013
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Fix bug wiggler rounding #1013
Conversation
|
@lfarv and @swhite2401 , atError is failing when used inside GWigSymplecticPass and GWigSymplecticRadPass methods. I have included it to avoid tracking a wiggler without energy because it is used latter in the integrator. For example, here Line 157 in 780657c
where pWig->Po has been previously set to:Line 40 in 780657c
Do you prefer that I remove the energy check ? Or to solve this error ? atintegrators/atelem.c:205:22: note: expanded from macro 'atError'
205 | #define atError(...) return (struct elem *) PyErr_Format(PyExc_ValueError, __VA_ARGS__) |
|
Dear @SebastienJoly , Wrt to the tracking of a single wiggler, I failed to understand how to modify atError to run macOS, it runs on linux but I can not test macOS locally, so, I will open a separate issue. |
|
@oscarxblanco: normally, to track a single element, you can use the However, it is useful to understand why |
|
@oscarxblanco I am afraid the problem remains. If I use the example in the issue, I get different tunes if the length of the last drift/undulator is equal to 0.425. But, I do obtain the exact same tunes if it is 0.4, 0.41, 0.42, 0.43, or even 0.435. Does it work with you? |
Oh , great ! Thank you, it works. >>> und.track(zin,energy=1e9)
array([[4.250e-06],
[1.000e-05],
[0.000e+00],
[0.000e+00],
[0.000e+00],
[2.125e-11]]) |
|
@oscarxblanco, @SebastienJoly, @swhite2401 : I think that we should be more strict. If the wiggler length is not a multiple of the period length, it is an error. With the rounding set here, there is still a problem: when tracking I would prefer raising an exception if |
Yes, it works for me. Here is my script, I get the same tune with a Drift of or a Wiggler. |
|
I am confused, I confirm that the tune difference still appear in your example. Moreover, I get a difference of the order of 1e-8 and 1e-13 for the first and last element of the list obtained from: |
|
@SebastienJoly , could you try again ? As requested by @lfarv I have added a check of the wiggler total length to period length ratio. It works on pyat, not yet in AT matlab. When I set the total length to 0.425 m, I get a difference in tune between the wiggler set Bmax=0 and a drift on the order of 10^-14, and a difference of 10^-22 or less on the tracking of a particle starting with an angle of 10e-6 rad. tune difference : [ 3.38618023e-15 -1.83741911e-14 nan]
track difference : [[-8.47032947e-22 0.00000000e+00 0.00000000e+00 0.00000000e+00
0.00000000e+00 -2.94036858e-25]] |
|
pyat implements the Ltot/Lw check at the python level so the check written in c on the pass method is redundant. For AT matlab I tested it and It works, and I think it is worth leaving the check at the c level because there are many cases when the lattice is saved as .mat and elements are used with any configuration they had a the time of creation of the file. |
|
Hi @SebastienJoly , Python 3.11.2 (main, Apr 28 2025, 14:11:48) [GCC 12.2.0]
at.__version__=0.7.2.dev56+g59a25cbd.d20251107
np.__version__=1.26.0 |
|
Dear @SebastienJoly and @lfarv ,
Now I'll wait for @SebastienJoly to confirm what I get with this modification before trying anything else. I think @SebastienJoly mentioned in #1012 to set LENGTH and NW directly in order to avoid problems with the lengths LENGTH/LW ratio. Unfortunately I think that would break compatibility or make the element a little too complicated (as is the case of the VariableThinMultipole, for example) because one would need to assure that redundant parameters are given the right order of priority and privilege one. But, in any case, if you consider this necessary it could be implemented. |
|
@oscarxblanco : If I understand, there is no more test at the C level, we rely on the tests at the creation of the element. That's enough for me, it just leaves the case of "wrong" wigglers store in .mat files… @SebastienJoly : you are right, defining the wiggler by its number of periods rather than the period length would eliminate the problem. But I agree with @oscarxblanco that it would either break the compatibility or make the element creation much more complicated. I can add that the present definition is based on the fact that in real life, a wiggler is usually defined by its period length ( U35, W240…) rather than by its number of periods… I can approve the PR, but I let @SebastienJoly decide ! |
lfarv
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
|
Dear @oscarxblanco @lfarv, |
|
Hi @SebastienJoly ,
gcc (Debian 12.2.0-14+deb12u1) 12.2.0
Linux pc542 6.1.0-40-amd64 #1 SMP PREEMPT_DYNAMIC Debian 6.1.153-1 (2025-09-20) x86_64 GNU/LinuxIn case it is the pass method,
static void GWigPass_2nd(struct gwig *pWig, double *X)
{
int Nstep = pWig->PN*(pWig->Nw);
double dl = pWig->Lw/(pWig->PN);
printf("Nstep=%d, dl=%.6g,PN=%d,Nw=%d,Lw=%.3g\n",Nstep,dl,pWig->PN,pWig->Nw,pWig->Lw);
for (int i = 1; i <= Nstep; i++) {
GWigMap_2nd(pWig, X, dl);
}
}You should see many lines, like this, confirming the parameters in the call to the integrator : Nstep=1000, dl=0.00045,PN=10,Nw=100,Lw=0.0045 |
|
Hi @oscarxblanco and here is the output I get To go one step further, this is what I get when I do I am on the git branch with your fix and use numpy 1.26.0 Unfortunately, I still do not have matlab. I will ask a colleague if he can run your tests. Do you need more information? |
|
Hi @SebastienJoly , there is still something we are doing differently. I get the correct tune, and a single particle tracking difference of 10^{-22}. See here below tune with drift [0.32536863 0.2625984 nan]
tune with wiggler Bmax=0 [0.32536863 0.2625984 nan]
[[4.250e-06]
[1.000e-05]
[0.000e+00]
[0.000e+00]
[0.000e+00]
[2.125e-11]]
Particle tracking difference : [[-8.47032947e-22 0.00000000e+00 0.00000000e+00 0.00000000e+00
0.00000000e+00 -2.94036858e-25]]With respect to the output you show, the last line says there is an error of 10^{-8} in the single particle tracking. I copy it here below for clarity I can reproduce your result if I use the master branch, where there is a mistake on the number of step during the integration. So, my suspicions are still about the installation or compilation of the pass method. Could you try to add one line to the pass method and check the output ? It is the line I wrote in a previous message. printf("Nstep=%d, dl=%.6g,PN=%d,Nw=%d,Lw=%.3g\n",Nstep,dl,pWig->PN,pWig->Nw,pWig->Lw);In that way we will know that the code you are editing is the one you execute, and we could see some of the integrator parameters. Also, you could also check that the Other than that, I have tested the installation with pip gpu flags, pip mpi flags, just pip inside my copy of the repo, and all of them give me the right tune. |
|
Hi @oscarxblanco For context, I have Can it be related to my gcc version? |
Very unlikely. are you using the dot ? do not use the -e version for the moment. cd my_at_copy/at
pip install .if you are working on a IDE, e.g. spyder, or jupyter, or conda, etc., you need to configure the IDE to use the virtual environment you prefer. Have you tried on a terminal ? (some_venv) $ deactivate # just needed if you have any venv activated by default
$ python -m venv new_venv
$ source new_venv/bin/activate
(new_venv) $ cd my_at_copy/at # move inside your copy
(new_venv) $ git pull
(new_venv) $ git checkout fix_bug_wiggler_rounding
(new_venv) $ pip install .
(new_venv) $ cd to_your_code_folder
(new_venv) $ python
(new_venv) >>> exec(open('the_code.py').read())and that is all. I hope it is complete enough as I am doing it from memory. |

Dear all,
this PR solves issue #1012 where there was in inconsistent behaviour of a wiggler set to Bmax=0 when compared to a drift of the same length.
The number of iterations is now calculated using
round. The user must take into account that the ratio between total wiggler length and the wiggle_period should be an integer.In addition, wiggler elements cannot be tracked alone. I include a check on the definition of gamma that is only set by the lattice, according to pull request #894 .