Skip to content

Conversation

GuySten
Copy link
Contributor

@GuySten GuySten commented Jun 17, 2025

Description

This pr relaxes some of the constraints on the input needed by random ray mode.

Continuous Energy Distributions:

In random ray mode continuous energy distributions are converted at runtime to discrete energy distributions according to the energy group boundaries.

More Spatial Distributions

Random rays are sampled from spatial probability distributions.
Currently only box shape is supported.
This pr implement halton sampling for more types of spatial probability distributions using a slightly more general algorithm.
Now random rays can be sampled in spherical and cylindrical geometry.

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)

@GuySten GuySten requested a review from jtramm as a code owner June 17, 2025 14:13
@GuySten GuySten changed the title relaxed random ray constaints relaxed random ray constraints Jun 17, 2025
@jtramm
Copy link
Contributor

jtramm commented Jun 17, 2025

Very cool - thanks for making this PR! This would indeed be super helpful for allowing accommodation of different energy distributions while we await PR #3114 to be updated/merged. Even once first collided source is merged in, this would still be useful in cases where we don't need to bother with a full FCS solve (e.g., if the source is spatially well distributed but still has a non-discrete energy distribution).

I had considered approaching this originally from a stochastic standpoint -- to just sample the user distributions up front randomly and then make a histogram of samples for the multigroup energy bins to determine the MG source spectrum. The upside to going the stochastic route is that very few lines of code would need to change, as we wouldn't need to add any additional functionality at all to the distribution code.

The analytical approach you've taken is faster/better, though the downside is that it introduces analytical integration functions for each distribution type that we'll have to validate during the PR process. As I'll need to verify somehow that the maths are all correct, perhaps you can share your work on this, or give a source? This also might be a good use case for using the C++ unit test capabilities to make tests for each of the new integral functions. Additionally, a regression test or two should definitely be added to exercise the new spectrum conversion code. Perhaps one regression test that has multiple sources, each with a different type of energy distribution?

Anyway, let me know your thoughts on the pros/cons of going the stochastic route instead! If we're in agreement on the analytical route, then I'll go through and make finer-grained comments once the testing/validation aspects have been added.

Another thing I've noticed up front -- there are a number of places where bare pointers are allocated with new. Typically in OpenMC we have avoided bare pointer allocation in favor of C++ containers (like vector), or usage of std::array if the size is known at runtime and storage scope/duration requirements are met.

@GuySten
Copy link
Contributor Author

GuySten commented Jun 17, 2025

I thought about your points about analytic vs stochastic route and I think another way is a hybrid approach. Implementing the stochastic method as the default method and overriding the implementation by the analytic route. That way we can have some distributions for which we do not compute the CDF for whatever reason and we can add a distribution and its CDF incrementally.

@GuySten GuySten marked this pull request as draft June 18, 2025 02:38
@GuySten GuySten marked this pull request as draft June 18, 2025 02:38
@GuySten
Copy link
Contributor Author

GuySten commented Jun 18, 2025

I am marking this as draft until I pass all tests and finish testing and bugfixes.

@GuySten GuySten force-pushed the discrete branch 2 times, most recently from 3248689 to 3823a67 Compare June 18, 2025 13:23
@GuySten
Copy link
Contributor Author

GuySten commented Jun 18, 2025

I integrated now the stochastic route and testing that uses it.
This pr will be ready when I will add the regression test with spherical shape and Mixture energy distribution.

I also converted raw pointers to iterators.

@jtramm, Do you have further comments?

@GuySten GuySten marked this pull request as ready for review June 19, 2025 14:14
@GuySten
Copy link
Contributor Author

GuySten commented Jun 19, 2025

This pr is ready now for review.

@GuySten GuySten force-pushed the discrete branch 2 times, most recently from 2b25fb3 to a81fd95 Compare June 26, 2025 11:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants