Repository navigation
Optimize Mapping of Random Ray Source Regions to Tallies - #3465
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR optimizes the mapping of random ray source regions to tallies to significantly reduce tallying time, particularly for large-scale simulations. The key changes include:
- Removing redundant resets of task containers during adjoint resets.
- Modifying the simulation to conditionally map source regions based on a mesh subdivision flag.
- Updating the source region to tally conversion logic to support a starting index, which aids in processing only new source regions.
- Adjusting the iteration method for tally mapping and updating the function signature and documentation accordingly.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/random_ray/source_region.cpp | Removed clearing loops for task containers during adjoint reset. |
| src/random_ray/random_ray_simulation.cpp | Added a condition to check mesh subdivision before mapping source regions and passing the starting index. |
| src/random_ray/flat_source_domain.cpp | Changed convert_source_regions_to_tallies to accept a starting index and iterated over all tallies using an index. |
| include/openmc/random_ray/flat_source_domain.h | Updated the function signature of convert_source_regions_to_tallies to require a starting index. |
Comments suppressed due to low confidence (4)
src/random_ray/source_region.cpp:261
- Please verify that the removal of the task_set clearing does not lead to stale or accumulated tasks in subsequent iterations.
MomentMatrix {0.0, 0.0, 0.0, 0.0, 0.0, 0.0});
src/random_ray/flat_source_domain.cpp:476
- Ensure that iterating over all tallies from model::tallies instead of model::active_tallies is the intended change and does not inadvertently include tallies that should be filtered out.
for (int i_tally = 0; i_tally < model::tallies.size(); i_tally++) {
include/openmc/random_ray/flat_source_domain.h:37
- Ensure the accompanying documentation clearly explains the purpose and expected value of the 'start_sr_id' parameter.
void convert_source_regions_to_tallies(int64_t start_sr_id);
src/random_ray/random_ray_simulation.cpp:512
- Confirm that adding the mesh_subdivision_enabled_ condition aligns with the intended behavior for handling mesh subdivisions and that mapping for these cases is managed separately.
!RandomRay::mesh_subdivision_enabled_) {
5 tasks done
paulromano
approved these changes
Jun 25, 2025
paulromano
left a comment
paulromano
left a comment
Contributor
There was a problem hiding this comment.
Nice optimization: simple but effective!
ahnaf-tahmid-chowdhury
pushed a commit
to ahnaf-tahmid-chowdhury/OpenMC
that referenced
this pull request
Jul 7, 2025
yardasol
pushed a commit
to yardasol/openmc
that referenced
this pull request
Dec 25, 2025
apingegno
pushed a commit
to apingegno/openmc
that referenced
this pull request
May 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Currently in OpenMC, at the end of a batch, if a new source region is discovered during transport then a search of all known source regions is done to relate them to any spatial tally filters. Source regions that have already been mapped to tallies will exit this process early, saving time. However, the exit process happens after a
Particleobject is allocated and searched for withexhaustive_find_cell(), making this mapping test become quite expensive.The present PR changes the behavior such that source regions are only mapped to tallies once. When mesh overlay is being used, this process occurs at the time when newly discovered source regions are moved out of the dynamic thread safe source region container into the permanent known source region container.
I had also thought about just adding in another data variable to the source region itself to use as a flag to indicate if it has been mapped to tallies yet or not. This would greatly speed up the exit process and save a lot of time. However, on large problems like JET we can easily have over 100 million source regions, so it's not a great design pattern to loop over all 100 million just to find one that needs mapping. Thus, the optimization proposed in this PR seems like the better long term route.
Note that this PR should not change any results from the code -- the same tallies will still be scored, but we are just reducing wasted effort.
Impact
I tested this new optimization out on a short run of the JET model on a large CPU node. The impacts are significant. Without this PR, tallying takes 55.78 seconds during the adjoint phase of the simulation. With this PR, tallying only takes 0.98 seconds (a 57x speedup for this section of the code).
Checklist