Give the remaining partial sort comparators a total order

[danielepanozzo]

Give the remaining partial sort comparators a total order

A sweep of every std::sort / std::unique / priority-queue comparator in src/wmtk and the components, looking for the failure this repository keeps hitting.

The bug class. std::sort is not stable. When a comparator treats two distinct elements as equivalent, the order it leaves them in is unspecified — and differs between libc++ (macOS), libstdc++ (Linux) and MSVC. In each case below that order reaches the output, so the same input produces a different mesh depending on the platform. This has already been hit four times before: TetMesh::get_edges, igl::sortrows in the vertex dedup, sort_edges_by_length, and the morton partition sort in #952.

Six sites

Site	What the comparator ignored	How the order reaches the output
TetMesh::get_edges	the tet id — every tet incident to an edge contributes an entry, so an edge shared by k tets appears k times	std::unique kept an arbitrary one, so which tet the returned tuple lives in was unspecified; get_edges feeds the operation queues
TetMeshTriangleInsertionConn old_face_vids	(tid, l_fid) of {v0,v1,v2,tid,l_fid}	the survivor is consumed as tuple_from_face(info[3], info[4]) — the discarded fields are the output
TetMeshTriangleInsertionConn new_face_vids	same	survivor and residual order become the face tuples handed to triangle_insertion_after
orig/EdgeSplitter.h cmp_es	v_ids, keeping only the edge length	split priority queue: equal-length edges popped in an unspecified order, which is the order they are split in
TopoOffsetTetMesh.h sort_edges_by_length	everything but the length	marching-tets split order → new vertex ids and the frontier list that labels offset tets
TopoOffsetTriMesh.h sort_edges_by_length	same (2D path)	same

Two things the sweep surfaced

Four of the six share one idiom: sort on a partial key, std::unique to collapse duplicates, then read the very fields the comparator ignored. It reads as correct — the grouping works and the deduplication works; only the choice of which duplicate survives is left to the standard library. Where the fix widens the sort comparator to the whole array, the unique predicate still compares the vids alone, so the grouping it relies on is unchanged and the wider comparison only orders within each group.

cmp_es had two siblings that were already right. cmp_ec (EdgeCollapser.h) and cmp_er (EdgeRemover.h) both tie-break on v_ids; only the splitter was missed. That is an oversight with a clear fingerprint rather than a deliberate choice.

Scope

~150 std::sort call sites were examined; six were changed. Sorts whose comparator is the default one over a fully ordered type (size_t, std::array<size_t,N>, std::pair) are deliberately left alone: equivalent elements there are indistinguishable, so no order over them is observable, and tie-breaking them would be noise. Two further sites were examined and left: unique_face_tuples already uses std::stable_sort (so its survivor is deterministic without a key), and unique_directed_edge_tuples is dead code — both lambdas throw on entry.

A note for whoever merges this with #949

The tie-breaks here are unconditional. On danielepanozzo/integration-test-hashes (#949), the equivalents for TetMesh::get_edges and sort_edges_by_length sit behind #ifdef WMTK_FP_STRICT — that macro does not exist on this stack at all. Those hunks will conflict textually, and the merge forces a policy decision: is a total-order comparator a correctness property (always on) or a reproducibility one (strict builds only)? The cost is one integer comparison on a tie; the symptom is a mesh that differs by platform.

Testing

51/51 unit tests and Integration_Tests pass.

Worth being explicit about what that does not show: these fixes are reasoned, not measured. The golden-hash tests are what would prove them, and they live on #949 — a different stack — while most of these paths are multithreaded-sensitive anyway. Integration_Tests only checks termination, so green means nothing regressed, not that determinism improved.

Stacked on #952.

🤖 Generated with Claude Code

[Copilot]

Pull request overview

This PR hardens several comparator call sites to ensure a total order (and therefore deterministic results) across standard library implementations, preventing platform-dependent behavior from std::sort / std::unique and priority queues in mesh processing.

Changes:

• Make TetMeshTriangleInsertionConn face-vid sorts total by comparing full std::array<size_t,5> keys.
• Add a deterministic tie-break to TetMesh::get_edges() so std::unique keeps a consistent representative tuple per edge.
• Add length-then-edge tie-breaks in topological offset edge sorting, and add a tie-break in TetWild’s edge-splitting queue comparator.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/wmtk/TetMeshTriangleInsertionConn.cpp	Ensures deterministic sorting of face-vid records so unique retains consistent representatives and order.
src/wmtk/TetMesh.cpp	Adds a Tuple tie-break to make edge representative selection deterministic across platforms.
components/topological_offset/.../TopoOffsetTriMesh.h	Makes edge-length sorting deterministic by breaking ties with simplex::Edge ordering.
components/topological_offset/.../TopoOffsetTetMesh.h	Same as above for the tet-mesh offset path, avoiding platform-dependent split ordering.
components/tetwild/.../orig/EdgeSplitter.h	Adds a deterministic tie-break for equal-weight edges in the splitter priority queue.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.