cpp/test/mesh/generation.cpp: build_tet cell-vertex order doesn't match hand-written expected tables
Nobody has claimed this yet.
Assessment
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Newbie friendliness
- 45/100
Research direction
Start with CHECK_adjacency_list_equal and the "Box tetrahedron mesh" case in cpp/test/mesh/generation.cpp, then trace build_tet in cpp/dolfinx/mesh/generation.h and the topology construction that reorders vertices. Re-enable the structural comparison and run the affected mesh-generation tests; done means establishing the ordering contract and making the test accurately enforce it without masking orientation issues.
Written by the indexing model from the issue text.
Description
Summary
cpp/test/mesh/generation.cpp has a CHECK_adjacency_list_equal test helper whose per-row structural comparison is commented out:
template <typename T>
void CHECK_adjacency_list_equal(
const dolfinx::graph::AdjacencyList<T>& adj_list,
const std::vector<std::vector<T>>& expected_list)
{
REQUIRE(static_cast<std::size_t>(adj_list.num_nodes())
== expected_list.size());
// for (T i = 0; i < adj_list.num_nodes(); i++)
// {
// CHECK_THAT(adj_list.links(i),
// Catch::Matchers::RangeEquals(expected_list[i]));
// }
}
So every one of its ~9 call sites across the file (interval, triangle left/right/crossed diagonals, hexahedron, tetrahedron) currently only checks num_nodes() — the number of rows — never the actual vertex lists. The hand-written expected tables (e.g. the tet Kuhn-decomposition cell-to-vertex list in the "Box tetrahedron mesh" case) are dead text and give no structural coverage.
Reproduction
Re-enable the loop (with std::int32_t i instead of T i, which doesn't compile as written) and rebuild:
for (std::int32_t i = 0; i < adj_list.num_nodes(); i++)
{
CHECK_THAT(adj_list.links(i),
Catch::Matchers::RangeEquals(expected_list[i]));
}
The "Box tetrahedron mesh" case then fails on the cell-to-vertex (connectivity(3, 0)) check, e.g.:
CHECK_THAT( adj_list.links(i), Catch::Matchers::RangeEquals(expected_list[i]) )
with expansion:
{ 0, 4, 1, 3 } elements are { 0, 1, 3, 4 }
CHECK_THAT( adj_list.links(i), Catch::Matchers::RangeEquals(expected_list[i]) )
with expansion:
{ 0, 5, 3, 2 } elements are { 0, 2, 5, 3 }
CHECK_THAT( adj_list.links(i), Catch::Matchers::RangeEquals(expected_list[i]) )
with expansion:
{ 0, 6, 4, 3 } elements are { 0, 4, 3, 6 }
CHECK_THAT( adj_list.links(i), Catch::Matchers::RangeEquals(expected_list[i]) )
with expansion:
{ 0, 7, 3, 5 } elements are { 0, 5, 7, 3 }
Each failure is a permutation of the same 4 vertices — never a wrong vertex set — for both float and double. The edge (connectivity(1,0)) and face (connectivity(2,0)) checks earlier in the same test case pass.
Why I didn't just fix it
Vertex order within a cell is geometrically meaningful (orientation), so this isn't obviously safe to paper over by rewriting the expected tables to match current output. But I also couldn't confirm it's a genuine build_tet bug, because:
Topologyconstruction re-numbers/re-orders cell vertices during distribution (even for a single-rankMPI_COMM_SELFmesh in this test), and- no ordering contract is documented for
connectivity(tdim, 0)anywhere I could find.
So this could be either:
- A real bug in
build_tet's Kuhn decomposition (cpp/dolfinx/mesh/generation.h), or - Expected-value tables that were written against raw
build_tetoutput and never updated after a later, legitimate reordering elsewhere in the topology-construction pipeline, or - Working as intended, with the row order simply never having been a documented guarantee — in which case the test itself is asserting something that shouldn't be asserted, and should compare vertex sets per row (or an orientation-aware equality) rather than exact sequences.
Where this came from
Found while working through the cpp/dolfinx/mesh/ code review test-gap appendix (re-enabling this exact check was one of its proposed items, F13-9). Verified against commit 44891b763603cf2c6b038597b031f2cb29155ce1 on main — build_tet and this test file are unmodified relative to that commit, so the repro applies directly to main.
- Dominant language
- C++
- Stars
- 1.2k
- Forks
- 265
- Avg merge
- 1d 19h
- Merged PRs (30d)
- 77
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
More from FEniCS/dolfinx
-
Difficulty 5/5 Over a week Newbie friendliness 35/100
-
Difficulty 4/5 3-5 days Newbie friendliness 45/100
-
Difficulty 3/5 1-2 days Newbie friendliness 74/100
-
enhancement
Difficulty 5/5 Over a week Newbie friendliness 25/100
-
Difficulty 3/5 1-2 days Newbie friendliness 68/100
Similar issues
-
Difficulty 2/5 1-3 hours Newbie friendliness 86/100
-
Sensor initialization takes very long when `--initial-sim-time` is set to current UNIX timestamp Open
Difficulty 2/5 1-3 hours Newbie friendliness 78/100
gazebosim/gz-sensors#662 · 1 comment ·
-
enhancement
Difficulty 2/5 1-3 hours Newbie friendliness 76/100
-
comp-datalake
Difficulty 2/5 1-3 hours Newbie friendliness 88/100
ClickHouse/ClickHouse#121222 ·
-
Difficulty 2/5 1-3 hours Newbie friendliness 68/100
LadybirdBrowser/ladybird#12123 ·