-
Notifications
You must be signed in to change notification settings - Fork 32
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
Correct Dipole tapering #623
Conversation
@lfarv , I had a quick look and it seems the C part is ready, could you please confirm? On the python side:
I the c part is confirmed to be ok I can take care of adapting the python interface. |
I will be happy to try it with the latest FCC lattice |
This branch provides a scaling of magnet strength by changing the reference momentum. So it's exact and includes all effects. There are 2 possibilities:
Ok for you to modify the python interface to act on dipoles. The second option is more difficult to handle (it needs inserting elements in the lattice), but it avoids acting on single elements. Do you think that a |
Ok I implement the python interface, I don't think this is necessary for multipoles. |
@swhite2401: The correct scaling is now done in For info: I had once again to modify the test accuracy, in the Matlab tests this time, and for the Windows platform. I still do not understand what is the reason for those unexpected changes which already occurred several times. |
This accuracy issue is really a mystery... it is true that I had to change these several times too... |
Tapering function has been adapted, results are identical for the tests I have done with the FCChh lattice. This is ready for review. |
I gave a quick look at this discussion and at the code. I admit that the name "Scaling" for a new Bending magnet attribute seems to me a bit too open to interpretation. May be it could be EnergyTaperingScaling? or TaperingScale, ... or whatever explains in a more immediate and concise way what is this parameter for. |
@simoneliuzzo : no, it represents a magnetic field scaling, you can use it for any purpose you like, not only for tapering ! |
Practically, this |
@lfarv, it is true that Scaling could need a bit more explanation. Maybe FieldScaling would be more self-explanatory? |
Ok for FieldScaling ! |
I make the modifications… |
Also FieldScaling is not so clear to me, but I imagine that I may be did not understand correctly what this scaling is doing to the field. It changes Brho? Why not EnergyScaling? Also,
|
@simoneliuzzo : the attribute changes the magnetic field of the dipole. For straight magnets, this is just a scaling of
So Then the @swhite2401 : in the doctoring of "Scales magnet strengths with local energy to cancel the closed orbit and optics errors due to synchrotron radiations." is very clear and describes exactly what happens. But the 2nd one: "The dipole bending angle is changed by changing the reference momentum." is may be responsible for the confusion reported by @simoneliuzzo. Is it really necessary to enter computational details? @simoneliuzzo again: you are right, we need the Matlab equivalent. Any volunteer? otherwise, I'll do it, if there is not too much urgency. |
Ok to remove this part. If you could do the matlab that would be nice... did you change the attribute name in the python tapering function as well? |
Yes, change is done in the |
The Matlab function @swhite2401 : from my experience with the Matlab function:
But this is based on results of a single lattice, so maybe it's not significant… |
Yes it is well possible, should we integrate the multipole in the iteration loop? It is probably the right time... default is 1 anyway so it won't impact existing codes |
I would say so. As it is now, if you don't ask for multipoles, |
Yes I agree! Please go ahead! |
There is a problem with the multipoles: unlike for dipoles, the scalings accumulate on multipoles, so that they cannot be in the iteration loop. So for now, they are taken out, but set before the loop on dipoles, so that they are taken into account. However it is a problem if the function is called twice on the same lattice: then it's wrong. The easiest solution would be to add the FieldScaling attribute on multipoles too. |
All pass methods now accept |
Here is a notebook checking that the tapering is now correct for dipoles, using either the |
A full test of the FieldScaling attribute is added, the branch is ready for merging |
Dear @lfarv and @swhite2401 I am very happy that a test/example is added. I hope it will find a place in AT itself (as for collective effects). To my understanding "tapering" is needed to adjust the magnets to the local energy of the beam, between two RF cavities.
(I attach a partial script I have for this, that needs modification) Could this be added? It would be in my opinion a more intuitive example for "tapering users". If FieldScaling has more potential than just correct optics due to large radiation effects, then examples of real use cases could be detailed for example via dedicated notebook tests (very appreciated) as the one proposed by @lfarv, or in the function help. I think that mixing tapering (energy loss due to radiation, s dependent) and off-momentum (global offset of energy) may lead to non immediate understanding for the users. I also have an operational comment. thank you for your help best regards
|
Dear @lfarv and @swhite2401, I run 2 cases, only in python for now. I removed all ExactPass methods I see a few differences. Most remarkably in this branch (dipole_tapering) the vertical dispersion with radiation seems slowly diverging. |
Dear @swhite2401 and @lfarv |
@simoneliuzzo : concerning tapering, this all looks good. Now for the differences between Matlab and python, they mostly appear without applying tapering (horizontal beta and eta), so it does not look related to this branch. It could interesting to look at that independently of this PR… In the vertical plane, closed orbit and dispersion seems to be in the numerical noise. Are there any elements in your lattice generating vertical orbit or dispersion? |
Dear @lfarv and @swhite2401 , here the two figures updated as requested. I see differences that are not related to this pull request so I do not feel confortable approving it. If for you those differences are irrelevant please approve, I will not oppose. |
@simoneliuzzo : Where do you see problematic differences? For me the tapering effect looks correct. The difference with the master branch is the correction of the dipole focusing effect when applying tapering. This focusing effect in dipoles appears for small bending radii, so I'm not surprised that with your lattice, it does not make a significant difference… |
Hello both, I am also more concerned about this lattice giving strange v orbit and dispersion... did you get a chance to check with MADX how it looks like? do you see the same with the exact passmethods? It would be nice to understand what the problem is. If you have the madx lattice available I can take a look. Tapering looks fine to me. |
Dear @swhite2401 and @lfarv, to my understanding tapering is supposed to have no effect in the vertical plane. So I expected no change in the vertical plane, while I see some (very small) changes. This is what (lightly) worries me. The modification done in this branch seem to have an effect on parameters that were supposed to be unaffected. We can check with MADX (M.Hofer at CERN has the files, but I bet there are small differences here and there, and it will take ages to get to exactly corresponding lattices and tracking). I personally do not see the interest here (may be for FCC studies) as it is unrelated to this pull request. For me the problem is simply: vertical should have not changed at all. Why does it change? Not to mention that I do not clearly see the added value of such large code changes (21 files changed! all passmethods!). Seen the changes in final optics provided by this pull request compared to the the previous "wrong" tapering function it does not seem really motivated. Let's also remember that FCC is unlikely to be realized and this effect has applications only for this lattice. Concerning the lattice:
For completeness I run the same figures for EBS (no vertical dipoles, no skew quadrupoles) in this case tapering reduces a bit the computed (extremely small) vertical dispersion. Also in this case, the computed vertical dispersion changes in dipole_tapering branch. By the way, this appears to me as the most visible change, thanks to the scales. |
Dear @lfarv and @swhite2401, @swhite2401 convinced me that small vertical dispersion changes could appear. I approve for the sake of moving forward to more relevant topics such as ExactPass. However I strongly encourage NOT to merge, as it is an overkill of code changes for no net final effect. Apparently the approximations done in the first version of the tapering functions (just few lines of easy to read code) where rather good approximations! |
@simoneliuzzo, @swhite2401 : If I understand, both lattices should give exactly zero vertical closed orbit and vertical dispersion. So what we get is just numerical noise (and its range is acceptable). You can also notice that it gets larger with the number of elements in the lattice. I do not see anything suspicious in these results. The problem seems to be in the Concerning the number of modifications, it's obviously due to the So if there is no other problem, I will indeed merge this branch to allow further work on C integrators. |
Somehow I was the author of this branch but in reality I am not so please go ahead |
@swhite2401 : why ? Any change since #623 (comment)? If the tapering is not useful, we have at least to remove the wrong one from the master branch. But I will anyway keep all the cleaning done in the the C integrators. |
Well you obviously took over! All I am saying is tapering is useful and used and this branch is fine for me, so please go ahead an merge it as is if you are happy with it |
Ah this is was missleading it ment -> I am not author, please go ahead with the merge |
@swhite2401 : complete misunderstanding ! Sorry, I understood that you were "not pleased" with the way the PR turned out… |
# By Laurent Farvacque (14) and others # Via GitHub * master: (28 commits) Add passive beamloading cavity (#586) Create BndStrMPoleSymplectic4RadPass (#665) Documentation fixes (#669) Update of the build process (#659) New Matlab function atsimplering (#657) Collective bugfix (#664) Correct the attribute name of solenoids in Matlab (#663) Error parsing args for twiss_in and r_4d (#662) Fix atmaincavities (#656) Fix attribute names in Simple Ring (#655) Remove collective passes from internal lattice_pass (#650) The DPStep keyword in linopt6 raises an error for 4D lattices (#651) Bug fix in atdisable_6d: keep the Energy field in cavities. (#654) fix: ring phase advances in computeRDT.m (#652) Correct the axis definition in plot_sigma (#648) Don't automatically cache the location of RF cavities (#640) Simple ring model (#643) Correct Dipole tapering (#623) Chromatic functions extended (#644) Repair the Matlab tests (#645) ... # Conflicts: # atmat/Contents.m # atmat/atphysics/Radiation/atdisable_6d.m # atmat/atphysics/Radiation/atenable_6d.m # atmat/lattice/at2str.m # atmat/pubtools/create_elems/atidtable_dat.m # pyat/at/lattice/elements.py # pyat/at/lattice/lattice_object.py # pyat/at/physics/matrix.py # pyat/at/physics/radiation.py # pyat/examples/CollectiveEffects/RobinsonInstability.py
This PR fixes issues with tapering method used until now that is making of polynomB[0].