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

Account for integral of underlying distributions when sampling Mixture #2658

Merged
merged 3 commits into from Aug 31, 2023

Conversation

paulromano
Copy link
Contributor

Description

The Mixture distribution type allows you to sample a mixture of multiple distributions at specified probabilities. We used to assume that the underlying distributions always integrated to one, but now we have distributions which are allowed to integrate to non-unity values to represent intensities (e.g., line spectra from a decay photon source). On the Python side, when sampling the Mixture distribution, the probabilities are weighted by the integrals of the underlying distributions, but we missed this on the C++ side. This PR fixes this so that the integrals of the distributions are accounted for when choosing which distribution to sampling from.

Checklist

  • I have performed a self-review of my own code
  • I have run clang-format on any C++ source files (if applicable)
  • I have followed the style guidelines for Python source files (if applicable)
  • [ ] I have made corresponding changes to the documentation (if applicable)
  • I have added tests that prove my fix is effective or that my feature works (if applicable)

@shimwell
Copy link
Member

oh wow good find, wondering if this will change the results of an R2S simulation slightly?

@paulromano
Copy link
Contributor Author

@shimwell Yes, this is actually how I found this bug in the first place. It affects results if you are using a chain file generated from ENDF/B-VIII.0 (which results in Mixture distributions for some decay photon spectra).

@shimwell
Copy link
Member

My c++ skills are not good enough to review this PR but I have been using the branch to run R2S simulations and can confirm that it ran without error if that helps :-)

Copy link
Contributor

@eepeterson eepeterson left a comment

Choose a reason for hiding this comment

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

Thanks for the fix @paulromano! The only thing I'm slightly concerned about is the meaning of the integral() of the distribution. It's a little hard to know what the meaning is when the actual integral of the probability density gives 1, but the integral() property does not. I know this is used to encode a sort of relative strength into probability mixtures when the integral of a distribution represents something like particles/cc/second, but I'm wondering if there is a better more consistent way.

src/distribution.cpp Show resolved Hide resolved
src/distribution.cpp Show resolved Hide resolved
src/distribution.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@eepeterson eepeterson left a comment

Choose a reason for hiding this comment

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

Good find and thanks for the fix @paulromano!

@eepeterson eepeterson merged commit c93feed into openmc-dev:develop Aug 31, 2023
18 checks passed
@paulromano paulromano deleted the mixture-sampling-fix branch September 1, 2023 02:10
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.

None yet

3 participants