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

External sources alias sampler #3201

Merged

Conversation

magnoxemo
Copy link
Contributor

@magnoxemo magnoxemo commented Nov 17, 2024

Description

Rather than doing a linear search over the CDF for sampling an external_sources_alias_sampler of DiscreteIndex class was defined in the source.cpp in the openmc model namespace. Which then gets the source strengths openmc::model::external_sources_alias_sampler.assign(source_strengths) and creates an alias table.

and a seed is passed through the model::external_sources_alias_sampler.sample(seed) method to get the index of the ith source.
This reduces the time complexity from O(n) to O(1)

Thanks to @gonuke for help and guidelines.

Fixes #3104

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)

@gonuke
Copy link
Contributor

gonuke commented Nov 17, 2024

Here are the results from some testing with 16 threads:

10^3 sources, 1e3 samples, original rate: 4.3e4 particles/s, accelerated rate: 3.3e4 particles/s
10^3 sources, 1e4 samples, original rate: 3.4e5 particles/s, accelerated rate: 4.8e5 particles/s
10^3 sources, 1e5 samples, original rate: 1.2e6 particles/s, accelerated rate: 2.4e6 particles/s
30^3 sources, 1e3 samples, original rate: 2.2e4 particles/s, accelerated rate: 4.4e4 particles/s
30^3 sources, 1e4 samples, original rate: 3.8e4 particles/s, accelerated rate: 5.1e5 particles/s
30^3 sources, 1e5 samples, original rate: 4.6e4 particles/s, accelerated rate: 3.4e6 particles/s
50^3 sources, 1e3 samples, original rate: 4.2e3 particles/s, accelerated rate: 4.2e4 particles/s
50^3 sources, 1e4 samples, original rate: 5.4e3 particles/s, accelerated rate: 4.2e5 particles/s
50^3 sources, 1e5 samples, original rate: 5.5e3 particles/s, accelerated rate: 2.5e6 particles/s
50^3 sources, 1e6 samples, original rate: ----- particles/s, accelerated rate: 5.4e6 particles/s
50^3 sources, 1e7 samples, original rate: ----- particles/s, accelerated rate: 2.9e6 particles/s
50^3 sources, 1e8 samples, original rate: ----- particles/s, accelerated rate: 3.4e6 particles/s

@gonuke
Copy link
Contributor

gonuke commented Nov 17, 2024

Is there a logical place to include scripts that we can use for timing? I don't think it makes sense for CI, and they probably don't make sense after a PR is adopted because it's hard to test the old code. What's the best practice?

Copy link
Contributor

@lewisgross1296 lewisgross1296 left a comment

Choose a reason for hiding this comment

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

If I understand correctly, the complexity for selecting the source is what goes O(N)->O(1). I see the testing varies number of sources and number of samples. What is the expected change in particles per second for varying the number of samples? It would be nice if the testing script printed the particles per second speedup as well.

Any idea why this case had a slow down?

10^3 sources, 1e3 samples, original rate: 4.3e4 particles/s, accelerated rate: 3.3e4 particles/s

It does seem that the cases with more sources and samples are accelerating more, so that is good.

@gonuke
Copy link
Contributor

gonuke commented Nov 20, 2024

The reason that there are changes for the number of samples is because with too few samples, the timing is affected by stochastic behavior and the particle rate will fluctuate with each time you perform the analysis. With the slower approach, the stochastic response goes away faster with number of samples because it simply takes longer. Because it's so fast, it requires many more samples to see the stochastic behavior converge with the faster approach.

@gonuke
Copy link
Contributor

gonuke commented Nov 21, 2024

Has this been processed with clang-format to ensure it meets the style guide?

@magnoxemo
Copy link
Contributor Author

Yes.

@gonuke
Copy link
Contributor

gonuke commented Nov 22, 2024

This will need to be coordinated with #3195 as they overlap in their impact. That other PR appears poised to be merged first - once that's done, we can update this for review.

@gonuke
Copy link
Contributor

gonuke commented Nov 22, 2024

@magnoxemo - please rebase this with #3195 for further review

pshriwise and others added 18 commits November 22, 2024 16:41
Co-authored-by: Patrick Shriwise <[email protected]>
Co-authored-by: Paul Romano <[email protected]>
…n openmc model namespace. ig it passes all the tests now
Copy link
Contributor

@paulromano paulromano left a comment

Choose a reason for hiding this comment

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

Thanks @magnoxemo! I fixed one thing on your branch -- we require separate logic for sampling when settings::uniform_source_sampling is true. It turns out that our existing tests didn't actually catch this case so I also added a test for this case.

@paulromano paulromano enabled auto-merge (squash) November 23, 2024 01:52
@gonuke
Copy link
Contributor

gonuke commented Nov 23, 2024

Thanks @magnoxemo! I fixed one thing on your branch -- we require separate logic for sampling when settings::uniform_source_sampling is true. It turns out that our existing tests didn't actually catch this case so I also added a test for this case.

Thanks @paulromano - we were going to tackle this update for settings::uniform_source_sampling but just building the DiscreteIndex differently (a list of uniform probabilities) but this works, too.

@paulromano paulromano merged commit 2d988a6 into openmc-dev:develop Nov 23, 2024
15 checks passed
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.

Improve algorithmic complexity of sampling among multiple sources
9 participants