atcollab / at

Accelerator Toolbox
Apache License 2.0
48 stars 31 forks source link

atplot off-energy #352

Open carmignani opened 2 years ago

carmignani commented 2 years ago

Hello,

I think the present version of atplot ignores the second input dpp, at least in matlab. It gives a warning and plots the optics at dpp=0. You probably can plot off energy optics by changing the RF frequency, but I don't know how to do if there is not an RF cavity.

I tried to plot the optics of a single ESRF cell, so no cavities, with dpp=0.01 and you get some warnings, but the plot is the on-energy one.

@simoneliuzzo tried the same with an old version of AT (tag AT 2.0) and there are no warnings and the plotted optics is off-energy.

Screenshot 2022-01-19 at 15 24 00

swhite2401 commented 2 years ago

@carmignani I have tried an the following seems to work for me:

load('S28F.mat');
ring = ARCA;
ring = atradoff(ring);

figure(1);
atplot(ring);
figure(2);
atplot(ring,0.01);

On energy: image

Off energy: image

You can see the tunes are different which seems to indicate dp was taken into account. Am I missing something? Can you share the full script that failed in your case?

simoneliuzzo commented 2 years ago

This are my observations figures:

AT2.0 (optics change without need for atrafoff):

Screenshot 2022-01-19 at 14 54 43 Screenshot 2022-01-19 at 14 54 43(1)

AT3.0 (optics do not change . Evidently we need an atradoff!):

Screenshot 2022-01-19 at 14 57 36(1) Screenshot 2022-01-19 at 14 57 36
swhite2401 commented 2 years ago

I am surprised you get a stable solution with radiations damping ON and no cavities...

In any case, with radiations ON (cavity or damping) you run through the 6D calculations that cannot accept a dp offset, the only way to act on the energy in this configuration is with an RF cavity.

I can see 3 options that would make this clearer for the user (there might be more): 1-add a clear error message saying that you have radiations ON an cannot use the dp input 2-Automatically set the RF frequency to nominal+delta_rf(dp) for the figure 3-automatically turn off radiations for the figure

The last one could make sense since beta functions and dispersion are in any case 4D quantities.

What do you think?

simoneliuzzo commented 2 years ago

unique(atgetfieldvalues(ARCA,atgetcells(ARCA,'PassMethod'),'PassMethod'))

ans =

5×1 cell array

{'BndMPoleSymplectic4Pass'}
{'CavityPass'             }
{'DriftPass'              }
{'IdentityPass'           }
{'StrMPoleSymplectic4Pass'}

those are the pass methods for the lattice I used for the plots. There is no radiation, but a cavity element. A lucky lattice.

The cavity was placed there to allow to have emittance values based on a single cell using atx(ARCA).

swhite2401 commented 2 years ago

Ok that explains it.

simoneliuzzo commented 2 years ago

For me this amplifies the problem.

dp is ignored in atplot not due to the radiation state, but due to the cavity in the lattice. To fix the plot, we have to run atradoff to actually inhibit the cavity.

Also, AT3.0 (that is how I personally call the present state of the repository) makes such that any lattice/function used before (not explicitly stating atradoff/on everywhere) is now deprecated. Likely not usable or not giving reproducible results. As if we suddenly moved from python2. to python3. . AT3.0 is better, but it makes anything written in AT2.0 obsolete. Also, not only the scripts, but the lattice files themselves become invalid without notice.

It happened already when we switched to AT2.0, but there was some more time to prepare.

lfarv commented 2 years ago

The present behaviour is similar to the one described by @swhite2401's point 1: if a non-zero dp value is provided, the warning message is:

"Warning: In 6D, "dp" and "dct" are ignored"

Which tells exactly what is happening.

The message could be lengthened to say: "To run off-momentum, change the RF frequency or use atradoff to turn off the longitudinal motion". Though it looks rather obvious to me. Both solutions are valid, as atlinopt6 is now able to extract all 4D parameters (betas and dispersion) even with longitudinal motion.

@simoneliuzzo: when using the old version, you indeed got modified values, but it was using atlinopt with radiation ON, which as you know may give completely wrong results (depending on the location of the RF cavity). Missing test…

For me, the present state is correct, but I'll be happy to improve the warning message if you think it' useful. Or turn the warning into an error…

swhite2401 commented 2 years ago

In fact this break many scripts and codes and this is not good, @lfarv I know the previous version was giving wrong results but abrupt changes even they represent major improvement make our lives miserable.... Do you think we could find a solution that would make the matlab AT backward compatible with the previous (wrong) version, such that corrective actions can be implemented gradually?

swhite2401 commented 2 years ago

Something like a general option use_atlinopt that would restore the old default but with deprecation warning for instance

simoneliuzzo commented 2 years ago

I have no objections to leave everything as it is, even if it makes my life more difficult. If the community has no objections, I will adapt. My only request would be to create a new TAG of AT stating in the release notes the updated constraints for lattices regarding cavity and radiation and that some old lattice files, with forbidden combinations of Cavity and radiation state, are not supported anymore. The message could also indicate to run (I think) ring=check_radiation(ring,'force',0) to fix them. A list of the functions that have "compulsory" radiation state from now on, could also be provided.

My largest regret with the recent upgrades is the fact that it will not be possible to run the same scripts radiation on or off, as it use to be possible in the past.

simoneliuzzo commented 2 years ago

may be an addiational function at_set_tracking_mode(ring,'4D'), at_set_tracking_mode(ring,'6D'), could replace the strange (to me) behaviour of atradon/off doing both rad off and cavity setting.
An equivalent at_get_tracking_mode(ring) could replace check_radiation, that in fact checks also the cavity state. This is may be only semantic but it could be of some help at least for me.

simoneliuzzo commented 2 years ago

I agree with @swhite2401 about the automatic frequency change if dp input is given for a 6D lattice. This may solve many "unexpected behaviours", when running very frequent computations such as for example detuning with momentum, or atplot off energy. It would restore some of the user-friendlyness baragained in change of exactness. This seems to me however less trivial than it seems, for example thinking of the ESRF booster lattices that is operated off-energy.

lfarv commented 2 years ago

The example of atlinopt is for me representative of a "soft" evolution: there is no new failure, it's now correct where in the past it was wrong, and there is a simple warning issued when a probable wrong usage is detected… Apart from a better warning, could it be better?

My largest regret with the recent upgrades is the fact that it will not be possible to run the same scripts radiation on or off, as it use to be possible in the past.

@simoneliuzzo, you are unfair: that's on the contrary what we are trying to do:

A major point is that we cannot deliberately leave known bugs in the master branch, which is now widely distributed through Matlab Central.

But there's a point where I agree with you: we are missing "Release notes" mentioning bug fixes and new constraints.

may be an addiational function at_set_tracking_mode(ring,'4D'), at_set_tracking_mode(ring,'6D'), could replace the strange (to me) behaviour of atradon/off doing both rad off and cavity setting.

I agree, the atradon function name is very bad: it does not involve only radiation, but more globally any source of energy variation. But if you're really concerned with backwards compatibility, that does not seem high priority.

lfarv commented 2 years ago

I agree with @swhite2401 about the automatic frequency change if dp input is given for a 6D lattice. This may solve many "unexpected behaviours", when running very frequent computations such as for example detuning with momentum, or atplot off energy. It would restore some of the user-friendlyness baragained in change of exactness. This seems to me however less trivial than it seems, for example thinking of the ESRF booster lattices that is operated off-energy.

I would not like that a function like atplot (for example) changes the lattice, where intuitively we expect it to just display it.

What can be done is to provide a function (atsetcavity) which sets the frequency according to a given dp or dct condition. That's what is done in python with the set_rf_frequency function.

Now about you examples (detuning with momentum and off-energy beta functions), these are pure 4D parameters, so they are much better looked at with a 4D lattice! Try to keep in mind the physics you are looking at, do not introduce longitudinal motion and expect that AT will silently remove it.

swhite2401 commented 2 years ago

@lfarv, this function is certainly very useful in any case and the warning message needs to be improved, we may think of: _"You have either radiation damping or an active cavity in your lattice, 6D calculations will ignore dp and dct inputs. To run off-momentum please use the RF frequency or use ring.radiationoff() to turn off synchrontron motion"

Now concerning additional new features, if users are suffering (and that seems to be the case) there is a problem with the code we cannot ignore this: My personal feeling is that a function requiring radiations off could turn them off internally (with potentially a warning) instead of calling check_radiations() and throwing errors. There is only one way to turn off radiations so there absolutely no issue there on the other hand new errors are very difficult to handle in complex codes (such as the ESRF operation tools). This would of course imply making a copy of the ring with radiations off and running the calculation on this copy, the lattice is not modified! In fact, (at least in python) this is already used in several function and could be extended to more of them.

The other way around is more tricky as turning ON radiations can be done with several options, so I have no better option than check_radiations() for this case.

carmignani commented 2 years ago

Hello,

my test was done with a single cell with radiation off, but the problem was the presence of a cavity element that I didn't notice.

I have a few comments about radon/radoff problems:

simoneliuzzo commented 2 years ago

I agree about the fact that atlinopt6 is a great improvement.

However it requires several actions by the user to profit of the upgrade as the existing code is not running the same way anymore. This is a problem for me, but I think also for other users.

This is destabilizing and must be known ahead for users updating their AT to the latest version.

Presently the code runs with a 6D lattice if dp is given to atlinopt and provides wrong results as the dp specified by the user via atlinopt is ignored.

Since running simulations with a 6D lattice and providing dp is forbidden, then personally, I feel forced to do one of the two following actions: 1) 6D option: (change the script/code/functions) modify the code to change dp extenally via the RF frequency and use atlinopt6. This is awkward and in my opinion not user friendly, mostly since the function has dp as a possible input. 2) 4D option: (change the input lattice) turn the input lattice to 4D

The second option is simpler, so that is what AT3.0 pushes me to do. Both cases require to modify something, this is why the upgrade AT2.0 to AT3.0 is in my opinion not transparent for the users.

I agree that now atlinopt6 "works in any case". But in fact all functions using atlinopt (<- no 6), that is really basically in all scripts, now practically, work only if the lattice is 4D.

Some possible modifications:

1a) An error in atlinopt stating: "please switch to 4D using: .. (the command to do it) .. " 1b) Either a deprecation warning to atlinopt: "Please stop running 6D + dp, rewrite your code to use atlinopt6 (mind the different output, adding 6 is not a sufficient change)"

2a) May be an additional flag could be specified for atlinopt: 'convert_dp_to_frequency_for_6D_lattice'. This flag would add to the existing frequency a df(dp) ? 2b) Else or additionally a df input in atlinopt6 to restore what could be done with atlinopt(r,dp)?

swhite2401 commented 2 years ago

@carmignani in fact names are confusing but a single function to turn on/off synchrotron motion and check it is present is necessary: if a cavity is active 4D approximation cannot be used. We could name this functions checksynchmotion() and synchmotionon/off() instead of atradon/off but this would clearly break even more existing codes...

@liuzzo 2a- sounds like a good idea to me: it is a good compromise between the automated change and do nothing as I requires action from user, again this change can be applied on a copy of the ring to avoid changing the lattice in-place

1a and 1b also seem to be required so that user knows he is doing something wrong

swhite2401 commented 2 years ago

@liuzzo: there is one ambiguity though: in 4D it is clear what the dp does... in 6D do you apply the frequency shift w.r.t. the nominal frequency or the one defined in the cavities?

simoneliuzzo commented 2 years ago

for me the one in the cavities, assuming it is set before via the various commands to do it either to nominal or to whatever.

simoneliuzzo commented 2 years ago

I must say that 2a) is for me a work around. it does not really solve the problem, as the code needs to be modified in any case, adding the flag. So it is as annoying as switching to atlinopt6 everything. It may be a quicker way to restore locally the old behavior.

2b) would bring back a feature removed (may be not intentionally) in the passage from atlinopt -> atlinopt6 and simplify a lot the switch from one to the other. [twi,t,c] = atlinopt(r,dp,refpts) -> [?, twi] = atlinopt6(r,df,refpts,'dp',...,'df_from_dp',true/false)

For me the most important remains: INFORM THE USERS about the CHANGES (AT3.0 release), TELL THEM THAT THE CHANGES ALREADY HAPPENED and that THEY WILL HAVE TO COPE WITH IT. Give help on how to migrate the code from the old way to the new one (allow them to decide not to migrate).

swhite2401 commented 2 years ago

For me the most important remains: INFORM THE USERS about the CHANGES (AT3.0 release), TELL THEM THAT THE CHANGES ALREADY HAPPENED and that THEY WILL HAVE TO COPE WITH IT. Give help on how to migrate the code from the old way to the new one (allow them to decide not to migrate).

I fully agree with this, especially for the matlab on which many machines rely for operation... For the rest it seem you would need in any way case some changes to deal with the new output format...

lfarv commented 2 years ago

I'll try to summarise a few points mentioned here and which can drive the next modifications:

  1. Warning if alinopt6 is called with dp and 6D lattice: @swhite2401's proposal is rather verbose, but that's ok for me, I'll implement it,
  2. atradon/atradoff: I already said, I fully agree with @carmignani that the names are misleading. But the functionality is the needed one: the discriminating point is the longitudinal motion, whether it comes from RF cavity or radiation. We need a global control. But we cannot change the names any more… The only thing we could do is renaming check_radiation as checksyncmotion: it's a recent function, it's mainly for AT's internal use, and too bad for those who already use it. I'm not pushing for that, but if you think it's worth…
  3. Off-momentum with 6D motion: I propose to add the setting of the off-momentum frequency for a given dp or dct in atsetcavity, as it can already set the nominal frequency. Running off-momentum a general feature, not restricted to atlinopt6, so I do not see any reason to implement that inside atlinopt6. atlinopt, atlinopt6, atplot, etc. should display what they are given as input, not modify it silently, even in a copy (anyway a function argument in Matlab is always a copy…). For @swhite2401 and @simoneliuzzo: there is no ambiguity in this computation: AT has a clear definition of the on-momentum tuning: it's the energy/frequency for which the path length is equal to the machine circumference (different from the "control room" definition where the average orbit is zero, we already discussed that).

All these are improvements, which do not break anything but do not solve the compatibility problems you encounter. For this:

Think of the proposed atlinopt modification: It should solve most of compatibility problems: nothing to do, correct results at the cost of a single warning. And to switch to atlinopt6, it's true it needs some modifications (interface and behaviour) but it is more flexible. Also do not use 6D unless you need it: keep in mind that most of the linear optics theory is 4D, and in AT 4D is much faster but correct.

swhite2401 commented 2 years ago

@lfarv these were just suggestions trying to help simone and nicola, long warning are in fact annoying... so please shorten it if you wish.

Concerning realease, some codes have prod and dev realeases, prod is a stable version with strict versioning and notes while the dev moves much faster and is maybe less documented.

Users are then free to attach to one or the other depending on their needs, shall we consider something similar to avoid the above issues in the future? I know this is more maintenance work but would shield users from ongoing developments...

simoneliuzzo commented 2 years ago

All is ok for me.

I would like to note that the changes bringing the decreased user-friendliness (from a user point of view, for what it is worth) have started to appear since 5th of May 2021, not since 3 years.

I would like that the dp argument of atlinopt(ring,dp) is kept. It is may be a not necessary feature to be able to ask for dp, but it is user-friendly.

I would also propose again that atlinopt6 includes the same user-friendliness, either via dp or via dfrequency if this is more appropriate, or in a wonderful world both cases (dp/p and dfrequency, the two "human readable" quantities, unlike dct). The lattice is modified only inside the function call and upon a clear, intentional, request (via the input) of the user.

In the release notes it should be mentioned that atlinopt is not backward compatible, as it will not accept badly written lattice files.

AT3.0 is my nick-name for the present master branch . It is a parallelism with python, as for me the present AT3.0 (master) requires the same effort of going from python2. to python3. (to obtain similar results we have to write different code).