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

Test for use of keep_lattice in find_orbit6(). #301

Merged
merged 6 commits into from
Sep 23, 2021
Merged

Conversation

willrogers
Copy link
Contributor

@willrogers willrogers commented Sep 21, 2021

This is how I expect to use the keep_lattice keyword argument, and it appears to be failing in pyat-0.2.0.

The second call (with keep_lattice=True) is returning nan in each element of the array.

Is this related to #299 ?

cc @MJGaughran

@willrogers
Copy link
Contributor Author

I see that this test starts failing in 4f21954.

Is this a valid test and something needs fixing? Or am I misunderstanding something?

@swhite2401
Copy link
Contributor

I think this is a problem that needs fixing, @lfarv, any idea?

@lfarv
Copy link
Contributor

lfarv commented Sep 22, 2021

@willrogers: what you try is exactly how it is supposed to work.

I see the problem, and strangely find_orbit6 is the only affect one: find_orbit4, find_m44, find_m66 work as expected.
So I am looking at the problem ! Thanks for the hint on the origin of the problem.

@willrogers
Copy link
Contributor Author

I have been debugging this morning.

The difference is calling get_timelag_fromU0() with ELossMethod.TRACKING instead of ELossMethod.INTEGRAL.

The problem is the call in energy_loss.py line 66:

        ringtmp = ring.radiation_off(dipole_pass=None,
                                     quadrupole_pass=None,
                                     wiggler_pass=None,
                                     sextupole_pass=None,
                                     octupole_pass=None,
                                     copy=True)

I think this call must be modifying the original lattice somehow.

@lfarv
Copy link
Contributor

lfarv commented Sep 22, 2021

@willrogers: well spotted, thanks. get_energy_loss does not change the original lattice, but it does track with a different one. So if it is used in find_orbit6 (case of no initial guess and method ELossMethod.TRACKING, I'll have to force keep_lattice to False. Not a big deal, but then you test is irrelevant. The best would the to change the test to use find_orbit4, where it makes sense, while I'll fix find_orbit6.

@willrogers
Copy link
Contributor Author

I added a warning if you pass keep_lattice when it is not valid. That might be a bit noisy so I can remove that commit if you like.

I update the find_orbit6() test and added one for find_orbit4().

@lfarv
Copy link
Contributor

lfarv commented Sep 23, 2021

I don't think the message is useful: keep_lattice is a hint given to try and save some time (not a lot, in fact…). How this is honoured in the function does not really matter since it does not impact the result at all. So I would remove the warning.

@willrogers willrogers merged commit d720f80 into master Sep 23, 2021
@willrogers willrogers deleted the keep-lattice-test branch September 23, 2021 12:37
@willrogers willrogers mentioned this pull request Sep 23, 2021
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.

3 participants