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

Momentum offset for 6d orbit #540

Merged
merged 4 commits into from
Feb 17, 2023
Merged

Momentum offset for 6d orbit #540

merged 4 commits into from
Feb 17, 2023

Conversation

swhite2401
Copy link
Contributor

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 to orbit.py and applied on find_orbit6 allowing to input dp, dct or df and shift the ring frequency internally, This is done n a copy of ring and does not modify the input lattice.

Drawback: the dp, dct or df 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 with dp argument is now identical to the one of find_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!

@lfarv
Copy link
Contributor

lfarv commented Dec 9, 2022

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…

@lcarver
Copy link
Contributor

lcarver commented Dec 9, 2022

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)

image

@lfarv
Copy link
Contributor

lfarv commented Dec 11, 2022

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:

Figure_2

(Example with the hmba.mat test lattice)

@lfarv
Copy link
Contributor

lfarv commented Dec 11, 2022

So we have to choose between 3 possibilities:

  1. Decorate everything (find_orbit and higher level functions) which may lead to >= 3 times the 6d check. Example linopt6 -> find_m66 -> find_orbit6,
  2. leave things as they are and fix get_chrom,
  3. decorate 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 ?

@swhite2401
Copy link
Contributor Author

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.

@lfarv
Copy link
Contributor

lfarv commented Dec 12, 2022

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…

@lfarv
Copy link
Contributor

lfarv commented Dec 12, 2022

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),

@swhite2401
Copy link
Contributor Author

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:

  • we pursue with this branch as discussed
  • we remove the arguments that cannot be used and decorate with check_6d(True)

Since the second option is what we add in the past and was not popular I would suggest to continue with this branch.

@swhite2401
Copy link
Contributor Author

Is it possible to decorate function in matlab?

@lfarv
Copy link
Contributor

lfarv commented Dec 13, 2022

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.

@lfarv
Copy link
Contributor

lfarv commented Dec 13, 2022

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.

@swhite2401
Copy link
Contributor Author

@lfarv, can I leave this to you? (I can do the python if needed but the matlab I am not comfortable with...)

@simoneliuzzo simoneliuzzo removed their request for review December 15, 2022 18:55
@swhite2401
Copy link
Contributor Author

@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.

@lfarv
Copy link
Contributor

lfarv commented Feb 17, 2023

@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.

@swhite2401
Copy link
Contributor Author

I merge the master and fixed conflicts, I think this is ready for merge. Better keep matlab and python separated on this one

@lfarv
Copy link
Contributor

lfarv commented Feb 17, 2023

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 ?

@swhite2401
Copy link
Contributor Author

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?

@lfarv
Copy link
Contributor

lfarv commented Feb 17, 2023

No, it just probably increase the number of commits, but no consequence on the result

@swhite2401 swhite2401 merged commit 66469ba into master Feb 17, 2023
@swhite2401 swhite2401 deleted the orbit6_dp_offset branch February 17, 2023 15:26
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

3 participants