cpp/test/mesh/generation.cpp: build_tet cell-vertex order doesn't match hand-written expected tables

Open
#4,522 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Assessment

Difficulty
4/5
Estimated time
3-5 days
Newbie friendliness
45/100
Issue type
Bug
Clarity
Mostly clear
Activity status
Active
Tech stack
cpp
Domain
testing

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:

  • Topology construction re-numbers/re-orders cell vertices during distribution (even for a single-rank MPI_COMM_SELF mesh in this test), and
  • no ordering contract is documented for connectivity(tdim, 0) anywhere I could find.

So this could be either:

  1. A real bug in build_tet's Kuhn decomposition (cpp/dolfinx/mesh/generation.h), or
  2. Expected-value tables that were written against raw build_tet output and never updated after a later, legitimate reordering elsewhere in the topology-construction pipeline, or
  3. 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 mainbuild_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

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from FEniCS/dolfinx

All issues in FEniCS/dolfinx

Similar issues

More C++ issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.