-
Notifications
You must be signed in to change notification settings - Fork 35
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 muon bremsstrahlung energy limits and sampling #1319
Conversation
- Remove erroneous min/max incident energy limits - Use cutoff energy in interactor - Add missing minimum cutoff energy limit - Fix sampling of the gamma energy
12d2f5f
to
b23d1f2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice fix and update! Just a couple of comments for future physics validations - may be it is a good time to add/run @stognini's validation suite for muon processes too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only error I see is the subtle one about the RNG; please feel free to merge once that's fixed!
sample_photon_energy_(rng), | ||
sample_costheta_(rng), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooh this can't be done reproducibly. There was once a hard to track discrepancy in our CI between intel/gcc because intel would sample one function first and GCC would sample the other.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I thought because I used brace initialization here they would always be sampled in the order they appear?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's only true for initializer lists, not constructor arguments... could be wrong though!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the standard:
4 Within the initializer-list of a braced-init-list, the initializer-clauses, including any that result from pack
expansions (14.5.3), are evaluated in the order in which they appear. That is, every value computation and
side effect associated with a given initializer-clause is sequenced before every value computation and side
effect associated with any initializer-clause that follows it in the comma-separated list of the initializer-list.
[ Note: This evaluation ordering holds regardless of the semantics of the initialization; for example, it applies
when the elements of the initializer-list are interpreted as arguments of a constructor call, even though
ordinarily there are no sequencing constraints on the arguments of a call. -- end note ]
So from the note it sounds like it should be true (I initially did make this mistake, but caught it when the SB and RB tests started failing).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, that's a deep cut. Good find! Hopefully the compilers all follow this standard 😅
This fixes a few things in the muon bremsstrahlung interactor:
I also slightly modified the brems final state helper so we can use it for this interactor too.