-
Notifications
You must be signed in to change notification settings - Fork 36
Matlab frequency control #560
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
Conversation
|
@simoneliuzzo : Any comment on this ? |
|
Dear @lfarv, I did not look at this PR yet. I will do it soon. |
|
Dear @lfarv, I think this will require quite some tests, and I do not yet know exactly which ones... Could we add (at least in pyAT) a test (those run at "git push" to the repository) for each of the functions mentioned above making sure off-energy tracking is correct? For example showing that the same results (parameters, optics, etc..) are produced for the:
|
|
@simoneliuzzo : I added a test checking that tunes, chromaticities and beta functions are identical in 4D and 6D when varying |
ffba28d to
31cb0a3
Compare
|
Rebased on master |
|
@simoneliuzzo : can you check that the implemented tests are good enough ? I intend to merge this week. |
|
Dear Laurent, I will likely not have time to test this week. May be an other reviewer could do this in time? best regards |
|
I need someone to approve this PR pending for more than 6 weeks now… |
|
@lfarv, for me the principle is ok since we implemented it in python. However, I am not a matlab user so I am not able to validate this part... is this blocking you for other developments? |
|
@swhite2401 : the |
|
Dear @lfarv, I am postponing testing this PR as It appears like a lot of work to do and I never feel like I have enough time in a row to do the job. There are 4 major functions, with 3 inputs to be tested each for at least 2 different optics and at least for the on- energy and off- energy case. It makes 4 * 3 * 2 * 2 = 48 cases to test. If all these cases could be included in the test then it would be easy to just verify that the tests are correct and approve the PR. |
|
@simoneliuzzo : I understand ! But the integrated test covers roughly what you mention, including 2 different off-momentum values (positive and negative), apart from the 2 different optics (I have only one with 6D capability: hmba). |
|
The test could be extended to also include off-energy lattices then? Also I see that the test looks only at dp. What about ct and df? |
|
@simoneliuzzo : The test compares the 4d and 6d lattices for 3 |
|
@simoneliuzzo : the test sequence now includes 6D orbit and optics tests varying Is this good enough for you ? |
9a3c890 to
9fa3a87
Compare
|
Dear @lfarv, the updated methods would work also for off-energy optics? We are recently frequently working with optics that are matched off-energy, with dp=1% set in the lattice with a frequency shift. Would the results be the same in pyAT and matlab AT also in this case, where the input lattice has already a shift in RF frequency? thank you |
|
@simoneliuzzo: This PR concerns the introduction of |
|
Dear @lfarv, I cloned and checkout this branch. I will try to use the new inputs and give you feedback. |
|
Dear @lfarv, I am running some tests. How to input dp, dct, df for findorbit6? I looked at the help but could not find out. I have just cloned a new AT and switched to matlab_frequency_control branch. |
|
Dear @lfarv, I figured out the input for findoribt6. I think the help should be updated as for atlinopt6. The warning issued when using dp, dct or df input always mentions dp. |
|
Dear @lfarv I wanted to check that for equivalent dp, df and dct the output of findorbit is the same When I look at the printed data this is the output: for df and dct the values are identical, while for dp, they are not (but almost). May be I do some error when computing the equivalent dct and df? best regards |
|
Dear @lfarv
and
in the first case the result is identical to the one when giving as input only dct or only df in the second case the result is a different orbit, not seen in any of the other cases. How are the 3 different inputs handled when requested together? May be such requests should not be possible for the user? |
|
Dear @lfarv, in the master branch the dp, dct and df inputs are ignored. I thought they would error for wrong input given as other matlab functions. This PR will solves this issue for the three inputs dp, dct and df, giving them a meaning. However there are an infinity of possible wrong input keywords. Using the standard matlab input parser this is easily achieved. Here the ouptut of a request for a non existing input |
|
Dear @lfarv not only for |
|
Dear @lfarv |
|
@simoneliuzzo: tanks for the feedback ! I'll answer one by one:
Right. I'll update the help !
The choice between warning and errors was discussed at length: see #373. The question is now solved ! |
Your computation is approximate because alpha is non linear. The correct computation is: function [dct,df]=dctdf(r4, dpp)
[frf,l]=atGetRingProperties(r4,'rf_frequency','cell_length');
[~,o0]=findorbit4(r4,dp=dpp);
o1=ringpass(r4, o0);
dct=o1(6);
df=-frf*dct/(l+dct);
endHere you get the exact path lengthening from tracking. The results are then much closer (though they will never be exactly identical). |
|
Update for my previous post: the r4 input in the function is |
Dear @lfarv, this function could be usefull for AT. As a common user, I always convert dp in df using the momentum compaction factor. |
As you mentioned, such an input is stupid, so you get a stupid answer… Seriously, the results depends on the following priority order: It would be better to forbid such duplicate contraints. But that's not limited to the functions modified in this PR. It can be a topic for another pull request |
I agree. Also as I mentioned in a previous message, matlab offers already the solution to this problem, but it would need a lot of work. |
Dear @lfarv using this function for the conversion all comparisons are correct. Only the help of findorbit6 is missing the description of the dp, df, dct arguments. |
The idea in this pull request is that this conversion should not be necessary any more: you can use the way you find the most convenient. There is a better implementation in hh=props_harmnumber(harmcell,cell_h);
if isfinite(df)
frev = frev + df/hh;
elseif isfinite(dct)
frev=frev * lcell/(lcell+dct);
elseif isfinite(dp)
% Find the path lengthening for dp
[~,rnorad]=check_radiation(ring,false,'force');
[~,orbitin]=findorbit4(rnorad,dp);
orbitout=ringpass(rnorad,orbitin);
dct=orbitout(6);
frev=frev * lcell/(lcell+dct);
end
frequency = hh * frev;And that's where the priority is defined if you give several keywords. But if you find this function useful, it can be cleaned up and added. |
Agreed. But the best way is not to use the Matlab parser: practically, in AT, a function "consumes" the keyword it needs and transmits the remaining ones to the sub functions it calls (example: atlinopt6 -> findm66 -> findorbit6). So the upper level function does not have the full list of authorised keywords. It would be the task of the lowest level functions to check in the end, after all keywords have been consumed, that the remaining list is empty. As you said: a lot of work… |
|
@simoneliuzzo : the help of Thank for your feedback: what you pointed out is out of the capabilities of automatic test sequences. Because of the new approval rules, you'll have to approve again after my commit… |
This applies to Matlab the modifications of #540: in 6D, the functions
atlinopt6,findm66,findorbit6andtunechromnow accept a non-zerodp,dctordfargument. The RF frequency will be temporarily modified to provide the desired off-momentum value.