Skip to content
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

allow fit chrom with radon #349

Merged
merged 4 commits into from
Jan 12, 2022
Merged

allow fit chrom with radon #349

merged 4 commits into from
Jan 12, 2022

Conversation

swhite2401
Copy link
Contributor

atfitchrom was requesting rad off in matlab: this is breaking ESRF operation tools!

This branch uses directly atlinopt6 to compute the chromaticity, rad off is not required anymore.
!!!Warning: with radiation ON the initial dp offset is ignored, frequency should be used instead!!!

For future developments, especially on matlab please make sure the implementation is fully backward compatible
because this has strong impact on operation tools relying on AT. New errors coming from check_radiation() are
difficult to handle if not foreseen

@@ -25,7 +25,7 @@
%
% See also ATFITTUNE

check_radiation(ring,false);
%check_radiation(ring,false);
[deltaP,varargin]=getoption(varargin,'DPStep');
[deltaS,varargin]=getoption(varargin,'HStep',1.0e-5);
[dpp,varargin]=getargs(varargin,0.0,'check',@(arg) isscalar(arg) && isnumeric(arg));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be

[dpp,varargin]=getargs(varargin,NaN,'check',@(arg) isscalar(arg) && isnumeric(arg));

to avoid warnings if dpp is provided in 6D

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah thanks! My matlab skills are very limited, would you have time to fix this? Also we found a bug in atenergy() that is still looking for the harmonic number on cavities.

We need these fixed for tomorrow for the restart....

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I take this PR, it's a single line to change. What about atenergy() ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you handle atenergy() as well?

@lfarv
Copy link
Contributor

lfarv commented Jan 12, 2022

New errors coming from check_radiation() are difficult to handle if not foreseen
I know it's a difficult problem. New checks are necessary to prevent wrong uses, as it has been the case for long. But these checks may even break AT itself. I have a dedicated branch for that, but it has been sleeping for long…

@lfarv lfarv self-requested a review January 12, 2022 14:09
@lfarv
Copy link
Contributor

lfarv commented Jan 12, 2022

About atenergy: I can look at it, but it's more tricky. Cavities still have a HarmonicNumber field (from the atrfcavity element creation function), even if it is not used in tracking any more. Did anything changed about that ?
I do not see any change in atenergy which may affect its behaviour. Can you tell me more about the problem ?

@swhite2401
Copy link
Contributor Author

Well the problem is that some function (atreduce?) are removing the HarmonicNumber form the cavities so atenergy returns a NaN

@lfarv
Copy link
Contributor

lfarv commented Jan 12, 2022

Ok, but I still do not understand why this field is now removed while it was (or is still) mandatory. Does you problem occur on the master branch, or did anything change in another branch ?
Anyway, I can compute harmnumber from the RF frequency, but I'd like to understand what is the cause of this new problem.

@swhite2401
Copy link
Contributor Author

@simoneliuzzo faced the issue I cannot tell you much more

@lfarv
Copy link
Contributor

lfarv commented Jan 12, 2022

I create a new branch for atenergy and set the computation based on Frf, but I keep it until the problem is identified.

For this PR, I don't have the possibility to test it, so I'm waiting for Simone and/or Nicola's approval.

@simoneliuzzo
Copy link
Contributor

Hi Laurent,

The harmonic number disappears after calling to atclean. I think this function is not part of AT. I keep looking with @carmignani .

thank you for your help!

ciao
Simone

@lfarv
Copy link
Contributor

lfarv commented Jan 12, 2022

Still get a crash, it's not ready

@lfarv
Copy link
Contributor

lfarv commented Jan 12, 2022

@simoneliuzzo: Did atclean change ? If true, that's understood, and I can provide in a few minutes a new version using the RF frequency.

Concerning this PR, there are still problems…

@simoneliuzzo
Copy link
Contributor

I think I found where to fix the HarmonicNumber disappeared. atclean looks for the fields output of [req,opt]=RFCavityPass
and harmonic number is not there anymore. So I added HarmNumber among the fiels to keep. now this is ok.

@lfarv
Copy link
Contributor

lfarv commented Jan 12, 2022

OK, clear. We're been too fast in removing it from the mandatory fields. Anyway, a revised atenergy is ready. New PR in a moment.

@lfarv
Copy link
Contributor

lfarv commented Jan 12, 2022

Ready for merging. It was more tricky than expected. atfitchrom was not meant to be used with radiation ON. Which is anyway a bad idea…
@simoneliuzzo and @carmignani : any remark before merging?

@simoneliuzzo
Copy link
Contributor

ok for me.

@lfarv lfarv merged commit ee723f8 into master Jan 12, 2022
@lfarv lfarv deleted the atfitchromrad branch January 12, 2022 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants