determinism: give the remaining partial sort comparators a total order (resurrects #953) - #980
Open
danielepanozzo wants to merge 1 commit into
Open
determinism: give the remaining partial sort comparators a total order (resurrects #953)#980danielepanozzo wants to merge 1 commit into
danielepanozzo wants to merge 1 commit into
Conversation
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: a
comparator that treats two *distinct* elements as equivalent. std::sort is not
stable, so the order it leaves such elements in is unspecified and differs between
libc++, libstdc++ and MSVC -- and in each case below that order reaches the output.
Six sites. Four share one idiom: sort on a partial key, std::unique to collapse
duplicates, and then read the very fields the comparator ignored. That reads as
correct, because the grouping and the deduplication both work; only the choice of
which duplicate survives is left to the standard library.
TetMesh::get_edges -- entries are (v0, v1, tuple) and every tet incident to an
edge contributes one, so an edge shared by k tets appears k times. The unique
below kept an arbitrary one, so *which tet the returned tuple lives in* was
unspecified, and get_edges feeds the operation queues. Tie-break on the tuple,
whose operator< already orders totally.
TetMeshTriangleInsertionConn, old_face_vids and new_face_vids -- arrays of
{v0,v1,v2,tid,l_fid} ordered on the vids alone. The survivor is consumed as
tuple_from_face(info[3], info[4]), so the discarded fields are the output.
Comparing the whole array keeps the vid grouping the unique relies on -- the
first three elements still dominate -- and adds (tid, l_fid) as the tie-break.
orig/EdgeSplitter.h, cmp_es -- the split priority queue ordered on edge length
and ignored v_ids, so equal-length edges, which symmetric input produces in
quantity, were popped in an unspecified order, and that is the order they are
split in. Its siblings cmp_ec and cmp_er already tie-break on v_ids; this one
was missed.
TopoOffsetTetMesh.h and TopoOffsetTriMesh.h, sort_edges_by_length -- ordered on
length alone, and the order is the marching-tets split order, so it decides new
vertex ids and the frontier vertex list that labels offset tets.
Sorts whose comparator is the default one over a fully ordered type are left alone:
equivalent elements there are indistinguishable, so no order over them is
observable. That covers the great majority of the ~150 call sites.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
(cherry picked from commit 5671370)
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
1 participant
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.
Resurrects #953, which is still OPEN but targets
danielepanozzo/concurrency-cleanup— a branch whose own PR #952 was closed as superseded by #934. With the base orphaned, #953 has had no path to main since 2026-07-20. Same stranded stack as #979.Cherry-picked from
5671370bba. Applies to current main with no conflicts — the commit is independent of the concurrency work beneath it in the original stack.What it fixes
A sweep of every
std::sort/std::unique/ priority-queue comparator insrc/wmtkand the components, looking for comparators that treat two distinct elements as equivalent.std::sortis not stable, so the order it leaves such elements in is unspecified and differs between libc++, libstdc++ and MSVC — and in each of these sites that order reaches the output.Six sites, four sharing one idiom: sort on a partial key,
std::uniqueto collapse duplicates, then read the very fields the comparator ignored. It reads as correct because the grouping and the deduplication both work — only the choice of which duplicate survives is left to the standard library.TetMesh::get_edges— every tet incident to an edge contributes an entry, so which tet the returned tuple lives in was unspecified.get_edgesfeeds the operation queues.TetMeshTriangleInsertionConn,old_face_vids/new_face_vids— the survivor is consumed astuple_from_face(info[3], info[4]), i.e. the discarded fields are the output.Each is fixed by extending the comparator to a total order, keeping the existing grouping so the
uniquestill collapses the same sets.This is the same class of bug as the nondeterminism I hit while diagnosing the tetwild cap divergence, where the post-insertion mesh differed run to run and decided whether the model converged or timed out. This commit does not address that one (that is parallel operation ordering, not comparator order), but it removes a related source of cross-platform divergence.
Verification
Full suite 96/96 in Release and Debug, macOS/arm64. No formatting changes needed.
🤖 Generated with Claude Code