Skip to content

Give element blocks one canonical order, and match physics by name - #321

Merged
cmhamel merged 1 commit into
mainfrom
fix-block-ordering
Jul 25, 2026
Merged

Give element blocks one canonical order, and match physics by name#321
cmhamel merged 1 commit into
mainfrom
fix-block-ordering

Conversation

@lxmota

@lxmota lxmota commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

The bug

Per-block mesh data lives in Dicts keyed by block name, while the block names themselves live in an ordered Vector. Dict iteration is hash order, so values(mesh.element_conns) and mesh.element_block_names are two different orderings of the same blocks — and the code that indexes blocks positionally was mixing them.

On a three-block mesh with blocks b1, b2, b3 holding 1, 2 and 3 elements:

block_names(fspace)          = ["b1", "b2", "b3"]     # file order
[num_elements(fspace, b)...] = [3, 2, 1]              # Dict order

Block b's name and block b's elements were different blocks. Three consequences:

  1. FunctionSpace took elem_conns and elem_id_maps from values(...) but block names and ref_fes from element_block_names. With mixed element types across blocks this pairs a block's connectivity with another block's reference element.
  2. BCBookKeeping / _setup_sideset numbered blocks with enumerate(values(mesh.element_id_maps)) and then used that number to index element_block_names, so a side set resolved to the wrong block name — and from there the wrong connectivity and reference element for its tractions.
  3. write_to_file wrote block names in file order but the blocks themselves in Dict order, pairing each block with another block's name on round-trip.

Two blocks is the boundary case where it happens not to bite: every multi-block fixture in the suite is two blocks named block_1/block_2, for which hash order equals file order. That is why 51k passing tests never saw it.

The fix

block_names(mesh) is now the single source of truth for what "block b" means, with block_conns(mesh) / block_id_maps(mesh) returning per-block data in that same order. Every positional use is routed through them, and block_names(fspace) exposes the same order downstream.

With one order established, physics and properties can be matched to blocks by name rather than by position. Parameters now permutes a supplied NamedTuple into block order and requires its keys to be exactly the block names. This resolves the standing # TODO re-arrange physics tuple to match fspaces when appropriate — previously a NamedTuple written in any other order silently gave each block another block's material, and nothing downstream could flag it, since a run with swapped materials converges perfectly well to the wrong answer.

Tests

test/TestBlockOrdering.jl builds a three-block fixture (via Exodus, at test time — no new binary in the repo) whose hash order and file order differ, with deliberately different element counts per block so a permutation shows up as a size and not just a name. It covers the mesh/function-space invariant, the NamedTuple permutation, and each rejection case.

Verified failing on main before the fix:

Expression: [num_elements(V, b) for b = 1:length(BLOCK_NAMES)] == BLOCK_SIZES
 Evaluated: [3, 2, 1] == [1, 2, 3]

Full suite green after: 51824 passed.

Behavior changes for downstream users

  • p.physics / p.properties are keyed by real block names instead of region_N placeholders. (region_N appeared nowhere else in this repo and nothing indexed by it, but it was observable.)
  • A physics/properties NamedTuple whose keys are not exactly the block names now raises BlockMismatchError instead of being accepted.
  • An unnamed Tuple is rejected rather than assumed to be in block order.

Version bumped to 0.15.0 on that account rather than a patch release.

Not addressed

The p_degree > 1 path in FunctionSpace still builds connectivity from values(create_higher_order_mesh(...)). That path is untested and already broken independently (create_higher_order_mesh destructures a Vector{String} as if it were an id => name pair, and keys a Dict{Symbol,...} with Strings), so straightening out only its ordering would have implied a fix it does not have.

Per-block mesh data lives in `Dict`s keyed by block name, while the block
names themselves live in an ordered `Vector`.  `Dict` iteration is hash
order, so `values(mesh.element_conns)` and `mesh.element_block_names` are
two different orderings of the same blocks, and code that indexes blocks
positionally was mixing them:

  * `FunctionSpace` took connectivity and element id maps from
    `values(...)` but block names and reference elements from
    `element_block_names`.  On a three block mesh named b1/b2/b3 with 1, 2
    and 3 elements, `block_names(fspace)` read ["b1","b2","b3"] while
    `num_elements(fspace, b)` read [3, 2, 1] -- block `b`'s name and block
    `b`'s elements were different blocks.

  * `BCBookKeeping` and `_setup_sideset` numbered blocks with
    `enumerate(values(mesh.element_id_maps))` and then used that number to
    index `element_block_names`, so a side set resolved to the wrong block
    name, and from there to the wrong connectivity and reference element.

  * `write_to_file` wrote block names in file order but the blocks
    themselves in `Dict` order, pairing each block with another's name.

Introduce `block_names(mesh)` as the single source of truth for what
"block b" means, with `block_conns` / `block_id_maps` returning per-block
data in that same order, and route every positional use through them.
`block_names(fspace)` exposes the same order downstream.

With one order established, `physics` and `properties` can be matched to
blocks by name instead of by position.  `Parameters` now permutes a
supplied `NamedTuple` into block order and requires its keys to be exactly
the block names, replacing the `TODO re-arrange physics tuple to match
fspaces` -- previously a `NamedTuple` written in any other order silently
gave each block another block's material, with nothing to flag it.  A
single physics/properties object is still shared across all blocks, but is
now keyed by the real block names rather than invented `region_N`
placeholders.

Nothing caught any of this because every multi-block fixture in the suite
has two blocks named block_1/block_2, whose hash order happens to equal
their file order.  `test/TestBlockOrdering.jl` adds a three block fixture
where it does not, with blocks of different sizes so a permutation shows
up as a size and not just a name.

Behavior changes for downstream users: `p.physics` / `p.properties` are
keyed by block name rather than `region_N`, and a `NamedTuple` whose keys
are not exactly the block names now raises `BlockMismatchError` instead of
being accepted.
@lxmota
lxmota force-pushed the fix-block-ordering branch from 1e45416 to fd58593 Compare July 25, 2026 09:02
@lxmota
lxmota requested a review from cmhamel July 25, 2026 09:04
@codecov

codecov Bot commented Jul 25, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 87.09677% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 69.07%. Comparing base (157760c) to head (fd58593).

Files with missing lines Patch % Lines
src/Parameters.jl 81.48% 5 Missing ⚠️
src/FunctionSpaces.jl 84.61% 2 Missing ⚠️
src/meshes/AMRMesh.jl 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #321      +/-   ##
==========================================
+ Coverage   68.99%   69.07%   +0.07%     
==========================================
  Files          55       55              
  Lines        6112     6134      +22     
==========================================
+ Hits         4217     4237      +20     
- Misses       1895     1897       +2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@cmhamel cmhamel left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for shoring up this buggy part of the package @lxmota!

No worries about the stuff you didn't address. That's dead code anyway and will be removed once I finalize #319 as an initial stab towards mixed space fem

@cmhamel
cmhamel merged commit d6c23fe into main Jul 25, 2026
10 of 13 checks passed
@cmhamel
cmhamel deleted the fix-block-ordering branch July 25, 2026 15:20
lxmota added a commit to Cthonios/Carina.jl that referenced this pull request Jul 26, 2026
FEC now guarantees a single block order (`block_names`), derived from the
mesh file rather than from `Dict` iteration, and matches `physics` /
`properties` to blocks by name.  Carina was deriving its own block order
from `keys(mesh.element_conns)`, which is hash order and disagrees with
FEC's for three or more blocks.

Material assignment itself was already safe, since Carina keys the physics
and properties NamedTuples by real block names and FEC now permutes them
into block order.  What was not safe was the startup ordering check, which
compared Carina's Dict-ordered list against the function space's
file-ordered one and would have failed a perfectly valid run on any mesh
with three or more blocks.

Requires FiniteElementContainers with `block_names`; do not merge before
Cthonios/FiniteElementContainers.jl#321.
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.

2 participants