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 Moller-Bhahba energy distribution #1138

Merged
merged 2 commits into from
Mar 4, 2024

Conversation

amandalund
Copy link
Contributor

This fixes a bug in the rejection loop of the Moller and Bhabha energy distributions. The plots below show the energy deposition in the TestEM3 geometry before and after the fix with 250 MeV electron primaries and only ionization enabled:
testem3-simple-calo-ioni-250MeV
testem3-simple-calo-ioni-250MeV-fix
(This does not, however, eliminate the discrepancy in the energy deposition with all physics enabled.)

@amandalund amandalund added bug Something isn't working physics Particles, processes, and stepping algorithms labels Mar 4, 2024
@stognini
Copy link
Member

stognini commented Mar 4, 2024

Thanks @amandalund for fixing that. Looks great.

Got a list of errors (all the same, from lines 192 to 208) on wildstyle while building it, which come from a recent PR:

celeritas/src/orange/orangeinp/ConvexSurfaceBuilder.cc:192:39: error: extra ; [-Werror=pedantic]
 CSB_INSTANTIATE(PlaneAligned<Axis::x>);

Aside from that trivial fix, all the tests pass here.

Copy link
Contributor

@whokion whokion left a comment

Choose a reason for hiding this comment

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

Excellent!

  • This bug is the similar one that was fixed in EPlusGG model for using 1-p vs. p. Probably it might be very difficult to catch as changing the rejection condition gives a little difference (just curious why? probably the reject envelop is very flat in p and close to the target probability distribution?)
  • As the longitudinal energy distribution (along z, layer number) is now an excellent agreement to that of Geant4, just curious how the lateral distribution (either x or y, or any r direction) looks like if it is easy to compare between celeritas and Geant4. I expect that they should agree very well, but still give a good reference for the lateral energy distribution when MSC is added later - of course, they are decoupled, shoul be verified independently, but may give us a good base line when we interpret the lateral distribution with all processes are tuned on.

Copy link
Member

@sethrj sethrj left a comment

Choose a reason for hiding this comment

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

Great catch @amandalund ! As Soon said , this is very similar to #888. I wonder if instead of BernoulliDistribution—which is the same signature as C++ <random> but not obvious—we should define RejectionSampler? Since a lot of times our } while (...); are formulated as rejections rather than acceptance.

The other thing that's striking is that the number of sampled RNGs in the broken rejection case was way higher before the fix. So maybe we should extend our sampling distribution tests and check the "expected samples" count to see if any others are unusually high.

@sethrj sethrj merged commit 64288cb into celeritas-project:develop Mar 4, 2024
20 of 21 checks passed
sethrj pushed a commit that referenced this pull request Mar 4, 2024
* Fix Moller-Bhabha energy distributions
* Update all the tests
@amandalund amandalund deleted the moller-bhabha-fix branch March 4, 2024 19:50
@sethrj
Copy link
Member

sethrj commented Aug 5, 2024

See #1354

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working physics Particles, processes, and stepping algorithms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants