-
Notifications
You must be signed in to change notification settings - Fork 598
Optionally compute bounding boxes in Mesh.material_volumes #3731
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
base: develop
Are you sure you want to change the base?
Conversation
| #endif | ||
| } | ||
|
|
||
| // Helper function equivalent to std::bit_cast in C++20 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be worth adding a note to our future selves
| // Helper function equivalent to std::bit_cast in C++20 | |
| // Helper function equivalent to std::bit_cast in C++20 | |
| // TODO refactor using std::atomic_ref https://en.cppreference.com/w/cpp/atomic/atomic_ref.html when moving to C++20 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There already is a comment down in the beginning of MaterialVolumes::add_volume that discusses using OpenMP, which would be my preferred approach:
https://github.com/paulromano/openmc/blob/dc515205c0a58a1910c017b394bd76619b8ffd15/src/mesh.cpp#L212-L215
Let me know if you'd like to see something additional
GuySten
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
@paulromano, do you think we should deprecate openmc.MeshSource in a follow-up PR?
|
No, I think @pshriwise @eepeterson did either of you want to review this before merging? |
Description
This PR adds an optional
bounding_boxesflag to the mesh material volume calculation so that each (mesh element, material) pair can also return an axis‑aligned bounding box. The primary implementation lives inMesh::material_volumes; it uses low‑level atomic compare‑exchange intrinsics to achieve thread‑safe min/max updates for bounding box expansion. In the future, when we are able to utilize newer features in OpenMP 5, a lot of the ugly atomic-related code can be significantly simplified.The main motivation for this feature is representing the decay photon source in mesh‑based R2S calculations. At present,
R2SManageruses aMeshSourceplus a domain constraint for each element‑material pair, which can be extremely inefficient when a material occupies only a tiny portion of an element. With bounding boxes available, the updated workflow here is to forgoMeshSourceand instead build aBoxsource per element‑material combination, using the computed bounding box to restrict sampling while still applying the material domain constraint.Checklist