Add threading to mesh::h, mesh::cell_normals and mesh::compute_midpoints - #4380
Draft
garth-wells wants to merge 8 commits into
Draft
Add threading to mesh::h, mesh::cell_normals and mesh::compute_midpoints#4380garth-wells wants to merge 8 commits into
garth-wells wants to merge 8 commits into
Conversation
These per-entity loops write to disjoint output slots with no
cross-iteration dependency, unlike sibling functions in this file which
already accept num_threads. Add a num_threads parameter (default 1) to
each, following the existing common::local_range + std::jthread pattern,
and expose it through the Python bindings and mesh.py convenience
wrappers, matching the create_entities/create_entity_permutations
precedent.
Verified bit-identical results (including all cell_normals cell-type
branches: interval, triangle, quadrilateral) across num_threads in
{1, 2, 3, 4, 8} and 1-3 MPI ranks.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Doxygen only enforces full @PARAM coverage on a function once at least one parameter is documented. Adding num_threads without documenting the pre-existing parameters tripped WARN_AS_ERROR in the docs build. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
michalhabera
reviewed
Aug 9, 2026
| }, | ||
| nb::arg("mesh"), nb::arg("dim"), nb::arg("entities")); | ||
| nb::arg("mesh"), nb::arg("dim"), nb::arg("entities"), | ||
| nb::arg("num_threads") = 1); |
Contributor
There was a problem hiding this comment.
We avoid defaults in the nanobind layer, and handle any defaults in the Python later.
garth-wells
marked this pull request as draft
August 9, 2026 12:01
jorgensd
reviewed
Aug 10, 2026
| { | ||
| std::span<const T, 3> p1(x.data() + 3 * e_vertices[j], 3); | ||
| h[e] = std::max(h[e], delta_norm(p0, p1)); | ||
| std::span<const T, 3> p0(x.data() + 3 * e_vertices[i], 3); |
jorgensd
reviewed
Aug 10, 2026
| std::span<const T, 3> p0(x.data() + 3 * e_vertices[i], 3); | ||
| for (std::size_t j = i + 1; j < e_vertices.size(); ++j) | ||
| { | ||
| std::span<const T, 3> p1(x.data() + 3 * e_vertices[j], 3); |
jorgensd
reviewed
Aug 10, 2026
| std::array vertices{geometry_entities[i * eshape1], | ||
| geometry_entities[i * eshape1 + 1]}; | ||
| std::array p | ||
| = {std::span<const T, 3>(x.data() + 3 * vertices[0], 3), |
jorgensd
reviewed
Aug 10, 2026
| [](auto x, auto y) { return x - y; }); | ||
|
|
||
| T norm = std::sqrt(t[0] * t[0] + t[1] * t[1]); | ||
| std::span<T, 3> ni(n.data() + 3 * i, 3); |
jorgensd
reviewed
Aug 10, 2026
|
|
||
| return n; | ||
| // TODO: check the quadrilateral case | ||
| return run_threaded( |
Member
There was a problem hiding this comment.
Same comment regarding subspans in this section.
jorgensd
reviewed
Aug 10, 2026
| // into x_mid_chunk. This is thread-safe. | ||
| auto compute_midpoints_chunk = [](std::span<const std::int32_t> e_to_g, | ||
| std::size_t num_xdofs_per_entity, | ||
| std::span<const T> x, std::span<T> x_mid) |
jorgensd
reviewed
Aug 10, 2026
| num_cells = mesh.topology.index_map(tdim).size_local | ||
| cells = np.arange(num_cells, dtype=np.int32) | ||
| h = _cpp.mesh.h(mesh._cpp_object, tdim, cells) | ||
| h = _cpp.mesh.h(mesh._cpp_object, tdim, cells, 1) |
Member
There was a problem hiding this comment.
Should we parametrize one of this tests over num threads to check the new code logic?
jorgensd
reviewed
Aug 10, 2026
| fdim = square.topology.dim - 1 | ||
| facets = locate_entities_boundary(square, fdim, left_side) | ||
| normals = cell_normals(square._cpp_object, fdim, facets) | ||
| normals = cell_normals(square._cpp_object, fdim, facets, 1) |
Member
There was a problem hiding this comment.
Parametrize of thread size (1, 2)?
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
3 participants
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.
Summary
num_threadsparameter (default 1) tomesh::h,mesh::cell_normals, andmesh::compute_midpointsincpp/dolfinx/mesh/utils.h.common::local_range+std::jthreadpattern used elsewhere in the mesh module.num_threadsthrough the nanobind wrapper (mesh.h) and themesh.pyconvenience wrappers (Mesh.h,compute_midpoints), matching the existingcreate_entities/create_entity_permutationsprecedent (nb::arg("num_threads") = 1at the binding layer, Python-level defaultnum_threads: int = 1).Test plan
ninjaclean build oflibdolfinxclang-format --dry-run --Werror/ruff check/ruff format --check/mypyall pass on touched filesmpirun -n 3cell_normalscell-type branches: interval, triangle, quadrilateral) acrossnum_threadsin {1, 2, 3, 4, 8} and 1-3 MPI ranks🤖 Generated with Claude Code