Closed swhite2401 closed 1 year ago
Hello @swhite2401
Moving the decorator to orbit.py
is a good thing. This will allow to decorate almost anything. But decorating find_orbit6
does not solve everything: we still need to decorate the high-level function (those involving matrix computations), because tracking around the closed orbit to get the matrix needs the modified ring with the right RF frequency. So we'll be piling up multiple checks. This can be avoided by keeping a non-decorated find_orbit6
for internal use and a decorated one for user access.
Or alternatively keep find_orbit6 non-decorated (with a warning or error, as you like), but modify find_orbit
to handle dp, dct and df. We just need to ensure that internally we use the specific functions rather that the general one (anyway it would not be wrong, just adding useless controls).
Then we would decorate linopt6
, not get_tune
(it uses linopt6
), but get_chrom
, to have the right central frequency…
Or go for the brute force solution: decorate everything. The penalty does not look so tremendous. Much less than the one for using a 6d lattice when it's not useful. We are just leaving out the pedagogic aspect: one should not use a 6d lattice to compute the chromaticity…
The changes that have been made have fixed the chromaticity function.
import numpy as np
import at
import matplotlib.pyplot as plt
def func(dp, fit):
val = np.zeros_like(dp)
for o in np.arange(len(fit)):
val += fit[o] * dp**o * np.math.factorial(o)
return val
ring = at.load_mat('/machfs/carver/Repo/EBSlattices/AT/S28F.mat', key='LOW_EMIT_RING_INJ')
(fitx, fity), dpa, qz = at.chromaticity(ring, dpm = 0.02, npoints=31, order=3)
The changes that have been made have fixed the chromaticity function.
No, they don't: get_chrom
does not crash any more, but dp
is still ignored for 6d lattices:
(Example with the hmba.mat
test lattice)
So we have to choose between 3 possibilities:
find_orbit
and higher level functions) which may lead to >= 3 times the 6d check. Example linopt6
-> find_m66
-> find_orbit6
,get_chrom
,find_orbit6
but keep a non-decorated version for internal use, thus avoiding duplicated tests. Note that this is equivalent to point 2, with some renaming and adding a decorated find_orbit
for users.Any preference ?
Hello, @lfarv is correct , I have overlooked findm66
.
For what concerns the fix, it is complicated am I cannot convince myself on the best option.
However, if we are allowing dp, dct, df
arguments for 6d functions (such as find_orbit6
or linopt6
) we have to make them useful otherwise it makes no sense. This is why I would be tempted to say that all 6d functions allowing for these arguments should be decorated which is option 1... it is true that this leads to many useless checks but I don't think this would impact performance.
This is why I would be tempted to say that all 6d functions allowing for these arguments should be decorated which is option 1
Not exactly: in all 3 options, the idea is to decorate all high level 6d functions (linopt6
...). Options 2 and 3 simply try to avoid duplicate checks by keeping private non-decorated low-level functions for internal use. If it's limited to keep an undecorated find_orbit6
, that's rather easy, but for longer call chains like get_something()
-> linopt6()
-> find_m66()
-> find_orbit6()
, that may get tricky.
So I finally agree that option 1 is the easiest solution…
So now, if we continue with this branch, we must decorate:
find_m66
(otherwise it's wrong in 6d with dp specified),get_chrom
to have dp taken into account (see above),get_tune
, which may call line_pass
, so needs the right RF frequency. Decorating the nested get_centroid
would work, but would modify the lattice twice, once for linopt6
and then for get_centroid
It looks useless for linopt6
, get_optics
and derivatives (all they do is processing the output of find_m66
, they don't care about the modified ring),
get_chrom
and get_w
acitivated in linopt6
will give inconsistent results because they shift the frequency around ring.rf_frequency
, also find_orbit6
would need to be decorated I believe.
Do we expect others? In radiation.py
for example?
I can see 2 options:
Since the second option is what we add in the past and was not popular I would suggest to continue with this branch.
Is it possible to decorate function in matlab?
also find_orbit6 would need to be decorated I believe.
Yes, you are right!
Is it possible to decorate function in matlab?
The decorator syntax does not exist, but there is a wrapper which does the equivalent: see wrapper6d.m
. But the problem is worse in Matlab, because checking 6d needs a scan of the whole lattice (no lattice object to store attributes). So avoiding unnecessary checks is important, and we'll probably have to keep private raw versions… Or finally store the radiation state in the RingParam element.
Do we expect others? In radiation.py for example?
Everything is correctly decorated with check_6d(True)
except get_radiation_integrals
. get_radiation_integrals
does not care of 6d true or false, but we already noticed that off-momentum computation misses the quadrupole contribution, so removing the dp
input makes sense.
@lfarv, can I leave this to you? (I can do the python if needed but the matlab I am not comfortable with...)
@lfarv I think I have decorate everything that needs decoration, could you check that I did not forget anything? One strange thing is that I had to change the tolerance on one test of linopt6...this test was ok on my local computer but failed in github, I have no explanation for that.
@swhite2401: that looks ok for PyAT. I have Matlab similar corrections ready. Should we put them together or in another PR (cleaner way; I think)?. The failure in Matlab tests has been corrected by Lee in the master branch, so no worry. I'll look at the conflicting files if you allow me.
I merge the master and fixed conflicts, I think this is ready for merge. Better keep matlab and python separated on this one
I had to change the tolerance on one test of linopt6.
It's not the 1st time this strange thing occurs. It's worrying because we don't understand why results are changing without any reason, even if the 1e-10 threshold instead of 1e-12 is still perfectly acceptable.
Question about your last commit: why don't you rebase instead of merging ?
Question about your last commit: why don't you rebase instead of merging ?
I have had some bad experience with rebase in the past and ended up merging in any case, so I took the habit (maybe bad?) of merging straight away. Is this to be avoided?
No, it just probably increase the number of commits, but no consequence on the result
This PR is proposal to answer #528 and other similar problems related to 6D optics/orbit calculations with input momentum offset.
The
frequency_control
decorator is moved toorbit.py
and applied onfind_orbit6
allowing to inputdp
,dct
ordf
and shift the ring frequency internally, This is done n a copy ofring
and does not modify the input lattice.Drawback: the
dp
,dct
ordf
offset are applied w.r.t to the NOMINAL lattice and therefore overwrite existing frequency shifts, this is safer as it prevents applying these offsets several times. The help was modified to explain this behavior.The behavior of
find_orbit6
withdp
argument is now identical to the one offind_orbit4
with the same input arguments, which is convenient for codes having to handle lattices without or without radiations.I know this has been a controversial question and we may decide to discard this proposal, however the present situation raising errors all the time is not satisfactory and needs to be improved. Any comments or ideas welcome!