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

Allow MOAB k-d tree to be configured #2976

Merged
merged 6 commits into from Apr 26, 2024

Conversation

paulromano
Copy link
Contributor

Description

If you have unstructured mesh tallies based on MOAB, OpenMC will utilize an adaptive k-d tree (also provided by MOAB) as a means of doing faster spatial searches in order to determine matching tally bins. I was seeing very slow performance for building the k-d tree for large meshes and after digging into it determined that it has to do with some default parameters that MOAB uses for building the k-d tree. MOAB allows you to change these parameters but we don't expose this currently through our API. This PR adds an options attribute to UnstructuredMesh that will get passed to MOAB when building the k-d tree. So for example, you could specify:

mesh = openmc.UnstructuredMesh('mesh.h5m', 'moab', options='MAX_DEPTH=20')

which would tell MOAB to use a maximum tree depth of 20.

Further Details

I've found that there are two main parameters that impact performance, memory use, and initialization time for MOAB's k-d tree:

  • PLANE_SET — this controls how planes are chosen to subdivide space in the k-d tree.
  • MAX_DEPTH — as you might guess, this controls the maximum depth of the tree. Once the maximum depth is reached, all remaining points/elements are put together in a leaf node (which means a search would have to go through them sequentially).

There are four different options for PLANE_SET numbered 0–3 and the default is 1, in which the range of values is split by multiple planes that are evenly spaced and then the one that most evenly subdivides the range is chosen. In my testing, using PLANE_SET=2, in which a plane centered on the median vertex is used as the dividing plane, results in better performance, less memory use, and lower initialization time.

For MAX_DEPTH, the default value is 30 which seems to be overkill for all the cases I've tried. In this PR, we now use a maximum depth of 20 unless the user specifies otherwise.

As one particular example, when I run the model from our unstructured mesh example with fuel pins, I get the following performance characteristics:

You can see that with a maximum depth of 30, the performance is actually worse than for lower values. I've found 20 to be a sweet spot in terms of getting close to maximum performance while keeping initialization time and memory use modest.

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

Interesting find Paul. Just wanted to ask if double down and embree makes a difference here.

@makeclean
Copy link
Contributor

No, Embree has no K-D tree support (IIRC) - the K-D tree is built on tetrahedra while Embree operates on triangles. It would be some development effort but I expect you could hijack Embree BVH by inserting the triangular faces of the tetrahedra into the various calls, but there would be a lot of book keeping going from the triangle data you intersect back to the tets you actually want to record the data on.

@pshriwise
Copy link
Contributor

No, Embree has no K-D tree support (IIRC) - the K-D tree is built on tetrahedra while Embree operates on triangles. It would be some development effort but I expect you could hijack Embree BVH by inserting the triangular faces of the tetrahedra into the various calls, but there would be a lot of book keeping going from the triangle data you intersect back to the tets you actually want to record the data on.

Spot on, @makeclean. This is how BVH's are used when ppl want to leverage RT cores on GPU for tetrahedral mesh.

@paulromano
Copy link
Contributor Author

To be clear — I think we are in need of a better solution for accelerating searches on UM. This PR is just a bandaid to improve the status quo for the time being 🩹 I'm working with a UM for a stellarator that has > 1M elements and the k-d tree build times (and memory use) are just horrendous. The changes here at least get me to a point where I can run it in a reasonable amount of time.

@gridley
Copy link
Contributor

gridley commented Apr 24, 2024

Sounds like it'd be better to just track the rays on the unstructured mesh!

docs/source/io_formats/tallies.rst Show resolved Hide resolved
include/openmc/mesh.h Outdated Show resolved Hide resolved
src/mesh.cpp Show resolved Hide resolved
Copy link
Contributor

@gridley gridley left a comment

Choose a reason for hiding this comment

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

Looks good!

Copy link
Contributor

@pshriwise pshriwise left a 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 as well, @paulromano! I'm hoping to put a solution for larger meshes in place soon.

@pshriwise pshriwise merged commit d1d37a5 into openmc-dev:develop Apr 26, 2024
18 checks passed
@paulromano paulromano deleted the kdtree-parameters branch April 26, 2024 11:52
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.

None yet

5 participants