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

turn off quantdiff in ohmi_envelope #840

Merged
merged 2 commits into from
Oct 21, 2024

Conversation

swhite2401
Copy link
Contributor

Since #737 ohmi_envelope exits with a segmentation fault when an element with a random number generation is present in the lattice (in the issue reported the QuantumDiffusion element).
This PR propose a quick fix to prevent this issue, however, there seem to be a more profound bug with random generators that could affect other function.

@swhite2401
Copy link
Contributor Author

The idea here is to allow users to continue working while preventing a wrong usage that is to call ohmi_envelope() with random generators elements.

Copy link
Contributor

@lfarv lfarv left a comment

Choose a reason for hiding this comment

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

I agree that the problem results from a more sever bug in random generators. I'll start looking at that.

The fix OK to avoid crashes. But removing quantum diffusion most likely removes all radiation effects, so the result is still probably irrelevant. It's indeed a wrong usage. Another possibility would be to test the passmethods and raise an error if there is any diffusion element.

Anyway, it's ok for me as it is, it's up to you…

@swhite2401
Copy link
Contributor Author

Why would it remove all radiation effects? We still have the rad passes and correct calculation no?

@lfarv
Copy link
Contributor

lfarv commented Oct 17, 2024

Why would it remove all radiation effects? We still have the rad passes and correct calculation no?

Ok for collective effects. But for radiation, it's either QuantDiffPass or RadPass. And very likely, it applies to all magnets (I don't understand why mixing both), So if you remove QuantDiffPass, you don't put RadPass instead, you just turn radiation off.

One could do the replacement automatically, but I'm reluctant to silently modify the user's choices… My preferred option is to warn the user that what he asks is wrong, and help him make the adequate corrections.

@swhite2401
Copy link
Contributor Author

Ah ok now I get it , in fact I had completely ingnore magnet quantdiff passes, and consisdered the QuantumDiffusion element only, but you are right we should cover all cases.

I am nevertheless a bit reluctant to raise an error because ohm_envelope is called in many other functions to get the beam emittance and this may break other parts of the code...

In this specific case I think that switching, or turning off passmethods is ok, the returned emittance value will be correct. I can make a decorator remove_random() or similar, it could be used in other places such as energy loss calculation from tracking

@lfarv
Copy link
Contributor

lfarv commented Oct 18, 2024

I am nevertheless a bit reluctant to raise an error because ohm_envelope is called in many other functions to get the beam emittance and this may break other parts of the code...

I don't see how an error if ohmi_envelope is called with QuantDiff elements could break anything: it's indeed an error, and anything derived from it is erroneous. The error is the safest solution !

Raising an error has a pedagogic interest: it recalls, or teaches, the user that computing any optics parameter with random elements does not make sense ! Otherwise, he may repeat the mistake for ever…

Practically, all optics functions are concerned. Many (including ohmi_envelope) are based on find_orbit and find_mxx. Those are the two functions which should be protected. But for efficiency, in the many cases where they are called in sequence, the check should be done only once. Maybe checking the presence of the orbit keyword may help: then it's not necessary to do the test.

But I'm afraid that step by step we are slowing down AT.

@lfarv
Copy link
Contributor

lfarv commented Oct 19, 2024

@swhite2401 : sorry, I was answering too fast ! In fact, all "4D" functions are already protected. So the problem only concerns find_orbit6 and find_m66. And their derivatives linopt6, ohmi_envelope

Having a full protection, for an event which should very rarely happen is complicated and seems too penalising for me. I'm definitely in favour of the simplest approach: either a test test on pass methods and an error, or the option in this PR as it is (already approved): turn off all random elements.

@swhite2401
Copy link
Contributor Author

Ok I agree, then let's merge this one, and if we have complaints we can envisage something else.

I merge as is? Any comments?

@lfarv
Copy link
Contributor

lfarv commented Oct 21, 2024

OK for me

@swhite2401 swhite2401 merged commit aa3ae2b into master Oct 21, 2024
37 checks passed
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.

2 participants