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

Fix on atfastring #314

Merged
merged 7 commits into from
Oct 13, 2021
Merged

Fix on atfastring #314

merged 7 commits into from
Oct 13, 2021

Conversation

lfarv
Copy link
Contributor

@lfarv lfarv commented Oct 4, 2021

Fix the bug reported in #305.

@lfarv lfarv added Matlab For Matlab/Octave AT code bug fix labels Oct 4, 2021
@lfarv lfarv requested a review from swhite2401 October 4, 2021 20:11
@lfarv
Copy link
Contributor Author

lfarv commented Oct 5, 2021

In fact, there was a bug on each side ! Though not optimised, it should be ok for the moment.

@lfarv
Copy link
Contributor Author

lfarv commented Oct 5, 2021

For the radiation version of the fast ring, the non-linear and diffusion element are swapped compared to the initial version and to the Matlab version. I think I remember this make a difference (on the equilibrium state) in the first tests we did a long time ago. Are you sure they can swapped ? Maybe @simoneliuzzo or @carmignani remember, I may be wrong…

@lfarv
Copy link
Contributor Author

lfarv commented Oct 5, 2021

@swhite2401 : line 61: what kind of error do you want to ignore ? It would be better to be more precise to avoid ignoring unexpected errors.

     try:
         qd_elem = gen_quantdiff_elem(merged_ring)
         fastring.append(qd_elem)
     except:
         pass

ibegs = get_refpts(merged_ring, 'xbeg')
iends = get_refpts(merged_ring, 'xend')
markers = numpy.sort(numpy.concatenate((ibegs, iends)))
with warnings.catch_warnings():
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this change is fine, however we generally use fond_orbit_sync for this kind of calculation without radiation. This is why I had suppressed the warning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, but in this case, dct=0 is strictly equivalent to dp=0 : it's the on-momentum orbit. And it's the default behaviour.

@swhite2401
Copy link
Contributor

@swhite2401 : line 61: what kind of error do you want to ignore ? It would be better to be more precise to avoid ignoring unexpected errors.

     try:
         qd_elem = gen_quantdiff_elem(merged_ring)
         fastring.append(qd_elem)
     except:
         pass

I think this was a simple way to manage the case where ring.radiation=True but damping is off -> cavity only

@swhite2401
Copy link
Contributor

For the radiation version of the fast ring, the non-linear and diffusion element are swapped compared to the initial version and to the Matlab version. I think I remember this make a difference (on the equilibrium state) in the first tests we did a long time ago. Are you sure they can swapped ? Maybe @simoneliuzzo or @carmignani remember, I may be wrong…

Why would this be an issue?

@lfarv
Copy link
Contributor Author

lfarv commented Oct 5, 2021

Why would this be an issue?

No idea. I just remember that the equilibrium phase-space when running a large population was different, but again I may be wrong. It could be tested again

@lfarv
Copy link
Contributor Author

lfarv commented Oct 5, 2021

I think this was a simple way to manage the case where ring.radiation=True but damping is off -> cavity only

Ok. Do you know what class of error this raises ? If this is not clear, I'll put except Exception: to remove the warning…

@swhite2401
Copy link
Contributor

I think this was a simple way to manage the case where ring.radiation=True but damping is off -> cavity only

Ok. Do you know what class of error this raises ? If this is not clear, I'll put except Exception: to remove the warning…

I am not quite sure, I think it is the cholesky transform complaining (ValueError?), I can check that later today, but what you propose is fine

@lfarv
Copy link
Contributor Author

lfarv commented Oct 5, 2021

Well this tool is not so stupid: except Exception: is still too broad… Anyway, that's good enough.

@swhite2401
Copy link
Contributor

Well this tool is not so stupid: except Exception: is still too broad… Anyway, that's good enough.

haha ok then I check later today and let you know

@swhite2401
Copy link
Contributor

I confirm this is a ValueError so we can improve the try/except block accordingly

@lfarv
Copy link
Contributor Author

lfarv commented Oct 12, 2021

The except clause is now more strict. OK for merging ?

@swhite2401
Copy link
Contributor

ok for me!

@lfarv lfarv merged commit 382122d into master Oct 13, 2021
@lfarv lfarv deleted the atfastring branch October 13, 2021 09:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug fix Matlab For Matlab/Octave AT code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants