-
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
ringparam always returns an emittance #583
Conversation
April's fool ? Are you seriously considering deliberately returning wrong results ? I fear what the consequences of generalising this idea could be… Seriously, what would you expect from such results, even for "compatibility" reasons ? @simoneliuzzo showed the error may be very large! And back to compatibility, before #511 there was NO 'dp' input argument in If "some user" has a real and urgent need for such a trick, you may consider making a fork, and urge him to carefully keep his results confidential… |
This function accepts off-momentum lattices: 6D with non nominal RF. So Let's keep this discussion correct please, this is not a joke or a trick. Now for the function there are obvious problems and they need to be fixed, this proposal is one solution, that is for me perfectly acceptable, but there may be others. I just try to help people that suffer from these 'bugfixes', so please if you have better ideas propose them and let's try to improve this with an open mind. If we don't you are in fact perfectly right: new forks risk to appear and users disappear... One question: |
I have nothing against a warning, but changing the output is a clearly non-backward compatible action. In my specific case the pull request #511 killed my code with an error. What I need to do is to compute the bunch length for non-zero current on a lattice with errors and frequency not to the nominal setting. Could you please advise on how to proceed with these computations in the correct way? In my opinion the problem is that the computation of the natural frequency of the SR is incorrect when using a lattice with errors. I use to set the correct frequency within my functions, but now this action returns an error concerning off-momentum that is clearly off scope in my case. thank you for your help |
By the way, PR #511 has been merged without the approval of any of the 3 requested reviewers. |
Alternatively we may call ohmi_envelop for 6D lattices, this would give the correct results also with errors, no approximation |
@swhite2401 , |
Well what I was suggesting was to integrate it in ringparam, I give it a try and let you know |
I would also like to add that non-backward compatible bug fixes such as the one in PR #511 would deserve in my opinion a TAG before the change is merged. This would allow user as me to freeze their code to this given tag. |
Hello, |
The nominal frequency is the frequency corresponding to the nominal path length. I does not depend on errors. If you have another definition, can you explicit it ?
You can always go back to any point and freeze you version by using the commit hash: a tag is just a name associated with a commit.
Why not, but be careful, this will not be backward compatible, unless you introduce new fields for |
@lfarv , I have already started my migration away from matlab AT about 1 year ago. I plan to abandon it completely by the end of 2023. The updates and bug fixes made in matlab AT imply modifications that are comparable to rewriting my code. So I started rewriting it in pyAT. I need bunch length with current, I use the function BL = atBunchLength(RING, I_bunch, Zn) . It is this function that calls ringpara, and should call atx instead. The tag is a way to inform users that from there on, there is something really different. Is not a simple commit. It is a peculiar commit, that needs to be remembered. best regards |
I fully agree with @carmignani: returning wrong results is not an option, and the only way to improve the present state is to add the quadrupole contribution to the integrals. |
Why do you want to use this function with a 6D lattice? This was allowed in the past, because ringpara allowed you to pass a 6D lattice and it was a bug. Now if you pass a 6D lattice ringpara computes the parameters correctly, except the emittance and the bunch length because the quadrupoles are not included in the integrals. For this specific problem, I would add an atradoff inside atBunchLength. So the lattice is converted to 4D and there are no problems. I don't think this would create any problem to existing codes. |
The nominal frequency for a lattice with errors could be the frequency related to the electron path length, not to the lattice circumference. For example using this function (from @lfarv, June 2016)
Simulations of 50 possible lattice surveys show consistently +2kHz change of frequency. The EBS operation frequency is in fact 2kHz larger (352174152) than the one computed for a perfect ring (352172152). |
@carmignani I have nothing against forcing atradoff or replacing ringpara with atx. |
@simoneliuzzo : going to PyAT is indeed a good option, but I don't think it makes a difference concerning bug fixes: every software has bugs, and each bug fix is by definition not backward compatible, you don't get the same result as before. And don't forget that python is evolving fast. Each python release has its set of deprecated features. For example, look at the python 3.11 release notes: There is one python version per year, here is the official schedule. AT is following the python release schedule. As much as possible, we try to keep as many old versions as possible, but by now, we already dropped support for python 2.7, 2.5 and 3.6. Python 3.7 is almost 5 years old, its end-of-life is scheduled on 27 June 2023. Not talking of numpy, which also has its own schedule So you are not finished with keeping your code up-to-date ! |
I think I wrote atBunchLength and at the time I was ignoring the fact that ringpara was giving wrong results in case of 6D lattice. |
@carmignani |
@carmignani ok thanks, you can use this branch, so you have the discussion history. Could you please also modify the warning to point the user to atx? |
I can use this branch, but I think we should revert the commits about ringpara. |
@carmignani in fact, no please leave this branch to me, I have an idea from a simple integration of the quadrupole contribution, I'll put it here |
@carmignani : very good solution. Then |
Ok, then if the quadrupole contribution will work, there will be no need to change atBunchLength. So I wait for you modification and then we can see if I have to update atBunchLength or not. |
Matlab and python implemented, this is the method that produced the figures above |
The matlab test are still failing... I have to figure this out |
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.
Very nice.
I have a question about the behaviour around theta == 0. If I understand, quadrupoles are always processed, and in the case of a very small closed orbit, their theta
may be very small but not zero. I hope that there is no numerical divergence when theta is approaching zero, otherwise we should check for (abs(theta) < epsilon)
rather than (theta == 0)
. To be checked.
Anyway, it's ok for me.
Does anybody knows where these smileys come from ? nice, but ok, I got it ! |
Yes in fact I wasn't so sure about this... I left it like this because 0.0 is forbidden as it would return NaN for I5. Very small values on the other hand should still give the correct result. But I will check that this does not diverge you are right. |
@lfarv, I have run scan down to 0.001Hz frequency shifts and everything looks stable to me, however just to be on the safe side I will put a minimum angle at 1.0e-7. I have a question, now that things are correctly handle should we restore the dp input for matlab? |
What do you mean ? |
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.
Yes again, no question this time !
Sorry forget this, I thought you had removed the dp input to atsummary and ringparam since offmomentum was wrong, but in fact not. Ok so I merge. |
After PR #511 many users scripts broke because NaNs were returned for lattices considered as off-energy. This prevented using this function on lattice with errors following an orbit correction, this is a source of complaints and major disturbance to users.
This PR proposes instead to always return the on-energy emittances and to modify the warning explicitly mentioned that the emittances returned are for the on-energy lattice. This restores backward compatibility while warning the user that he might be doing something wrong.