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

Improve clipping of Mixture distributions #3154

Merged
merged 8 commits into from
Oct 11, 2024

Conversation

paulromano
Copy link
Contributor

Description

This PR adds an additional check in the Mixture.clip method for when the probability of one of the contained distributions is negligibly small. I've run into this when running shutdown dose rate calculations where an isotope at an extremely low concentration produces discrete lines in a photon spectrum that can safely be ignored.

Checklist

  • I have performed a self-review of my own code
  • I have run clang-format (version 15) 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

shimwell commented Oct 4, 2024

This looks very handy for speeding up the shut down dose simulations. If I've understood this correctly then clip previously removed just gamma energies that were low probability. Now it removes two slightly different things, low probability gamma energies and low probability distributions. Perhaps the check for removing the distribution should not be on the probability of the distribution itself but if all the gamma probabilities (perhaps scaled by distribution probability) within the distribution are below the threshold? Then we have a single value that controls things consistently instead of a value that could apply to gamma probabilities or distribution probabilities. I might have got lost here with all the probabilities.

@paulromano
Copy link
Contributor Author

@shimwell Correct, two slightly different things. The latter case (low probability distribution) is really targeted to handle the case where the distribution is not discrete and so it's not just a matter of checking the intensity. I ran into this where you can have a mixture of a Discrete and Tabular distributions. This was the best way I could think of to handle that but let me know if you have a better idea.

@shimwell
Copy link
Member

shimwell commented Oct 4, 2024

Perhaps just adding two args, one for distribution clipping one for probability clipping. Not sure if it helps but perhaps worth considering

@openmc-dev openmc-dev deleted a comment from cjie33311001 Oct 4, 2024
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 this @paulromano and sorry for the delay. Just a couple questions I'd like to sort out before merging.

openmc/stats/univariate.py Outdated Show resolved Hide resolved
openmc/stats/univariate.py Outdated Show resolved Hide resolved
Comment on lines +303 to +304
mix_clip = mix.clip(1e-3)
assert mix_clip.distribution == [d_large]
Copy link
Contributor

Choose a reason for hiding this comment

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

only question here is if inplace=False by default should we expect to get back the same Univariate objects in the resulting clipped Mixture? My initial reaction was that I would expect the underlying distributions to be different objects. I don't think any users will really care about this, but it could have side-effects if someone then modified d_large after mixing, not expecting it to effect the Mixture distribution, because there is a specific inplace kwarg to that effect in clip.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's an interesting thought. We could default to copying any distribution that is not a discrete distribution when inplace=False, although that could be a bit wasteful from a memory perspective. I don't have a strong feeling either way since I don't think this will be a common issue, so let me know which way you prefer.

Copy link
Contributor

Choose a reason for hiding this comment

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

me neither we can leave as is

openmc/stats/univariate.py Outdated Show resolved Hide resolved
@shimwell
Copy link
Member

Perhaps just adding two args, one for distribution clipping one for probability clipping. Not sure if it helps but perhaps worth considering

Or a single argument that can be a single float or tuple of two floats

openmc/stats/univariate.py Outdated Show resolved Hide resolved
openmc/stats/univariate.py Outdated Show resolved Hide resolved
@paulromano
Copy link
Contributor Author

@eepeterson Thanks for the suggestions; I've just updated the branch to incorporate them.

@eepeterson eepeterson merged commit 04ecf54 into openmc-dev:develop Oct 11, 2024
16 checks passed
@paulromano paulromano deleted the mixture-clip branch October 11, 2024 11:51
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.

3 participants