refactor(pangraph): derive block and node ids from genome names

[mmolari]

Follow-up to #197, which introduced the merge command.

This PR introduces a simplification to the code, removing the need to re-id block and nodes to avoid hash collisions.

Node and block ids were initially seeded from FASTA record numerical index (1,2,3...). This created potential hash collisions when merging different graphs and required re-indexing the blocks.

PR #197 introduced the requirement for all FASTA record names to be non-empty and unique. This creates a more elegant solution to our problem: include the path name instead of numerical index in the hashes, so that id collisions are avoided. This also has a very nice side effect: removes order dependence. In fact before this PR node and block ids depended on the order of the sequences in the fasta file.

Also fixes a pre-existing bug this exposed: alignment hits were collected in thread completion order (par_bridge), so repeated runs on the same input could produce different graphs.

[ivan-aksamentov]

⚠️ AI-generated content below. Verify all claims.

#199. refactor(pangraph): derive block and node ids from genome names

• Head: e2a26222c319b4082a1f75b758b55c6c4a1e0922 on refactor/name-derived-ids
• Base: feat/merge
• Stacked on: #197
• Scope: seeds block and node ids from the genome name instead of the FASTA record index, replaces merge-time relabeling with a path-id-only renumber_paths, and fixes a par_bridge alignment-ordering non-determinism.

Overview

Click to expand

Block and node ids are now derived from a hash of the genome name rather than from the input record index, so two independently built graphs are id-disjoint by construction and merge only has to renumber path ids. The change removes Pangraph::relabel, Pangraph::make_disjoint_from, and PangraphBlock::relabel, and adds PangraphNode::with_derived_id, PangraphPath::seed, Pangraph::renumber_paths, and Pangraph::path_id_upper_bound. A side effect is order-independence: block and node ids no longer depend on FASTA ordering. A separate commit replaces par_bridge with an indexed parallel iterator in the minimap2 kernel to make alignment output reproducible. The change spans the graph model, the merge command, the circularization path, and the id-derivation tests.

Observed

The diff matches its description: ids are name-seeded, the relabeling machinery is deleted, and merge renumbers path ids only. The build succeeds and the full test suite passes (see Validation summary). The disjointness-by-construction invariant holds and its name-uniqueness precondition is enforced (N2). One area needs a decision: the PR body states that name-derived ids mean "id collisions are avoided", and the code doc says they "cannot collide" [src]. That is a probabilistic property of a 64-bit hash, not a certainty, and the build path has no guard for the residual case (F1). The par_bridge fix is acknowledged in the PR description and is handled under Author-tracked (F12).

Blocking issues

One correctness item where a documented guarantee exceeds what the code provides.

🔴 F1. A genome-name hash collision within a build is unguarded, and the docs state it cannot happen [click to expand]

Pangraph::singleton seeds both the block id and the node id from id(&fasta.seq_name) [src]. build guards only against string-equal names (check_sequence_names), not against two distinct names hashing to the same 64-bit value. The internal merge_graphs used during a build has no disjointness check, and graph_join resolves a key conflict with a bare panic!("Conflicting key: '{kl}'") [src]. So a name-hash collision surfaces, if the two colliding singletons are ever joined, as a process-terminating panic naming neither genome, or as a silently overwritten BTreeMap entry (a genome disappears from the graph). The previous scheme used the record index, which is unique by construction, and make_disjoint_from tolerated collisions by re-derivation.

The doc comments assert the opposite: is_id_disjoint_from states disjointness "holds by construction for graphs written by pangraph 1.4 or later" [src], and merge_run states name-derived ids "cannot collide" [src].

Effect: for n distinct names the birthday bound gives a collision probability near n^2 / 2^65, tiny but nonzero, and the change turns a previously handled event into an unhandled crash or silent data loss with no diagnostic. The "cannot collide" framing also risks a future maintainer removing the merge-time is_id_disjoint_from guard, which would extend the panic to the merge path as well.

Suggestions:

• After constructing the singletons (or after each graph_join), assert the block and node count equals the genome count and report a clear error naming both colliding genomes, restoring the invariant as a checked precondition.
• Reword the docs to describe disjointness as overwhelmingly probable given distinct names, with the runtime is_id_disjoint_from guard as the residual-case backstop.
• Add a test that seeds two singletons whose names collide under id (mine two colliding strings offline, or inject a stub id) and asserts a descriptive error rather than a panic.

Non-blocking issues

Latent robustness, test, and convention items. Fix if time allows.

🟡 F2. Two node-id derivation schemes coexist; the unnamed-path path reintroduces order dependence [click to expand]

PangraphNode::with_derived_id derives the id from the genome seed [src], but the public PangraphNode::new(None, ..) still derives from path_id [src], which renumber_paths deliberately shifts. PangraphPath::seed compounds this: it returns self.id.0 for unnamed paths [src], exactly the quantity renumber_paths rewrites, and mixes a full hash and a small sequential integer through one usize. The precondition that paths are always named is enforced at three call sites, not by seed or the constructor.

Effect: production callers are migrated to with_derived_id, so this is currently latent. A future caller that reaches for new(None, ..), or any refactor that reweaves an unnamed path before validating names, silently produces order-dependent, renumber-unstable ids, defeating the disjointness foundation with no error.

Suggestions:

• Route all content-derived node ids through one function; make new take an explicit NodeId and remove the Option derivation.
• Make the name precondition a typed error at the derivation site (parse-don't-validate) so an unnamed path cannot produce a node id silently.

🟡 F3. `renumber_paths` dropped the reference-consistency checks `relabel` performed [click to expand]

The removed relabel / PangraphBlock::relabel validated, as a side effect of remapping, that every node referenced an existing block and every block alignment referenced an existing node. The replacement renumber_paths validates only the node-to-path reference [src]. merge_cmd_preliminary_checks checks non-empty graphs and name uniqueness but not internal reference consistency, and sanity_check runs only under if args.verify and #[cfg(debug_assertions)] [src].

Effect: a malformed input graph (node to missing block, or block alignment to missing node) that previously produced a clear error during merge can now reach merge_graphs in a release build without --verify, where it may panic on a missing key or produce a wrong graph.

Suggestions:

• Call sanity_check on both input graphs unconditionally in merge_cmd_preliminary_checks (it is O(n) integer/key comparisons).
• Or have renumber_paths (or a small dedicated validator) re-assert node-to-block and block-to-node consistency.

🟡 F4. Id uniqueness now depends on a 64-bit hash truncated to `usize` [click to expand]

id() returns hasher.finish() as usize [src]. On a 32-bit target usize is 32 bits, so the hash is truncated: the birthday bound for the F1 collision tightens from ~2^32 to ~2^16 genomes, and the serialized ids differ across platforms. This is pre-existing behavior, but the refactor makes it correctness-relevant, since ids are now the identity foundation and are written into the JSON output.

Effect: on a 32-bit build, materially higher (still small) collision probability and reproducibility drift across platforms. No effect on 64-bit targets, but nothing asserts the target width.

Suggestions:

• Return the full u64 from id() and widen NodeId/BlockId, fixing the id width regardless of target.
• Or, if only 64-bit targets are supported, assert size_of::<usize>() == 8 or document the target contract.

🟡 F5. Simplify golden node-id constants are SUT-derived, and `graph.nodes` is unasserted [click to expand]

const NID11/NID12 are 64-bit XxHash64 outputs [src] that can only be produced by running with_derived_id; the doc comment describes the derivation conceptually, but a conceptual description is not an independent oracle. test_simplify keys expected_graph and the block alignments by these literals, so a regression in the id-derivation formula moves the expected side in lockstep, and the test never asserts graph.nodes.

Effect: the whole-graph equality cannot detect a change in the id-derivation inputs; the concatenated node map is unverified in the simplify path.

Suggestions:

• Recompute the expected keys in-test via PangraphNode::with_derived_id(BlockId(1), &pathA, Forward, (0, 64)) (and (0, 60) for pathB), so the expectation is a function of named inputs and moves with a legitimate change while failing on an accidental one.
• Assert graph.nodes in test_simplify.

🟡 F6. The new id-derivation surface lacks direct tests [click to expand]

PangraphNode::with_derived_id has no direct unit test: its name-seeded behavior is only integration-covered (through a full build on a real dataset), while the slice.rs and merge_blocks.rs unit tests reach it only through unnamed paths, where seed() falls back to path.id.0 and reproduces the old formula. PangraphPath::seed has no direct test of the named-vs-unnamed selection. renumber_paths order-preservation is asserted only for two-path graphs, so a rank-mapping bug over three or more paths would pass. The two_genome_graph fixture also re-derives ids inline via id(name.to_owned()) [src], duplicating the production formula in singleton, so the id-disjointness tests would keep passing on the parallel fixture implementation even if production stopped seeding ids from the name.

Effect: the core order-independence mechanism has no fast, isolated regression guard, and one fixture forms a parallel untested implementation of the seeding rule.

Suggestions:

• Add unit tests for with_derived_id (same name gives equal ids, different name gives distinct ids, stored path_id equals the passed path's id) and for seed (named seeds from id(name) independent of path id; unnamed seeds from self.id.0).
• Add a 3+ path renumber_paths order-preservation test, and derive the fixture's ids from Pangraph::singleton so it cannot drift.

🟡 F7. Merge collision error misattributes a genuine hash collision to legacy graphs [click to expand]

The collision error attributes any shared id to pre-1.4 graphs and instructs "Rebuild the input graphs with the current version" [src]. Preliminary checks already reject shared names, so a collision at this point is either an old-version graph or a true 64-bit hash collision between distinct names. For the latter, rebuilding reproduces the same name-derived ids and the same collision, so the advice is unactionable.

Effect: a user hitting a real hash collision receives misleading advice and cannot resolve the failure by following it.

Suggestions:

• State both causes and give the actionable step for each (rebuild a pre-1.4 graph, or rename one of the colliding genomes), and report which ids collided where feasible.

🟡 F8. Hardcoded unreleased version `1.4` in a user-facing error and a doc comment [click to expand]

The merge collision error states "Identifiers are derived from genome names since version 1.4" [src], and the same 1.4 appears in a doc comment [src]. The workspace version is still 1.3.0 and the CHANGELOG carries this under ## Unreleased.

Effect: if the next release is not numbered 1.4.0, both statements become wrong and the error misdirects the user to a version that never introduced the behavior. The literal is duplicated across two sites.

Suggestions:

• Drop the version from the error and describe the cause behaviorally (ids derived from names vs. input order).
• Or single-source the version from the crate constant and set it only once the release number is decided.

🔵 F9. `path.seed()` re-hashes the genome name per node on the reweave hot path [click to expand]

with_derived_id calls path.seed() [src], which hashes the full genome name string on every call [src] with no memoization. It runs once per aligned node of every split block on every merge iteration (block_slice [src], and again in find_node_pairings during circularization [src]). The previous code hashed a PathId integer.

Effect: redundant string hashing whose count scales with total node count; small next to the per-block consensus copy, but reducible.

Suggestions: memoize the seed once per path (a non-serialized OnceLock<usize>), or thread a precomputed seed into a constructor variant.

🔵 F10. Robustness and test-scaffolding nits [click to expand]

• Pangraph::singleton stores BlockId(seed) and NodeId(seed) with the identical seed [src], so a singleton's block and node carry the same integer. Safe today because ids are compared within kind; a latent footgun for future code assuming disjoint id ranges.
• itest_merge_rejects_graphs_with_colliding_identifiers asserts only a message substring, not the error type [src].
• test_renumber_paths_keeps_graph_consistent asserts the invariant inside a nested loop [src], losing which node failed on a mismatch.
• The blocks_by_genome test helper keys on path.name().clone().unwrap_or_default() [src], collapsing any unnamed path to the empty-string key; latent since the test data is always named.

Suggestions: derive the two singleton ids from distinguished inputs if the equal-integer assumption is undesirable; assert the error type; collect offending pairs before asserting; key the helper on PathId or assert names are present.

🔵 F11. Prose nits [click to expand]

The merge error message joins two clauses with a semicolon [src]; the CHANGELOG entry ends in a hollow participial clause ("enforcing order-independence") that restates the prior clause [src]; the renumber_paths doc paragraph packs six technical referents into one sentence group [src]; and the generated docs/docs/reference.md carries file-wide em dashes from the help generator.

Suggestions: split the semicolon into two sentences; drop the participial tail; split the doc paragraph; normalize the reference generator output rather than hand-editing the file.

Author-tracked

Items the author documented in the PR description alongside the change.

🟡 F12. Alignment output non-determinism fix (acknowledged) [click to expand]

Author documented: the PR description states this branch "fixes a pre-existing bug this exposed: alignment hits were collected in thread completion order (par_bridge), so repeated runs on the same input could produce different graphs."

Author's assessment: the fix replaces par_bridge() with collect_vec().into_par_iter() so the parallel iterator is indexed and results keep input order [src].

Reviewer assessment: the fix is correct and complete. filter_matches uses a stable sorted_by_key(OrderedFloat(energy)) and greedily accepts equal-energy hits in pre-sort order, so a nondeterministic hit order changed which of two equal-energy matches was accepted; the indexed iterator restores input order and closes that path. Two additions:

• No regression test guards it. itest_build_ids_do_not_depend_on_input_order compares two different input orders, not repeated runs of one order, so a return to unordered collection would not reliably fail it. A test that builds a graph twice from the same input at several thread counts and asserts byte-identical output would have gone red before this fix.
• The commit is typed refactor although it changes observable output; under conventional commits a determinism fix is fix, and bundling it with the id-derivation change reduces bisectability.
• Unchanged, adjacent: the per-worker Minimap2Mapper::new(&idx).unwrap() on the same line panics instead of propagating the FFI error with context; rayon re-raises it, so it neither races nor deadlocks, but the error message is lost.

Validation summary

Validation checks [click to expand]

• ./dev/docker/run ./dev/dev l (clippy + build): Pass
• ./dev/docker/run ./dev/dev t: 388 passed, 0 failed
• Disjointness-by-construction invariant and its name-uniqueness precondition traced across all minting sites: Pass
• Removed methods (relabel, make_disjoint_from, PangraphBlock::relabel) leave no dangling references: Pass
• Determinism regression test present for the par_bridge fix: Fail (F12)

Notes

Click to expand

• N1. Net reduction in code smells: the deleted make_disjoint_from removes a fixed-count retry-until-pass loop (MAX_ATTEMPTS = 8), replaced by construction-time disjointness. The old methods are deleted outright rather than kept as adapters.
• N2. The disjointness invariant was traced across every production minting site (singleton id(name), sliced blocks id((parent, interval)), detached blocks id((node_id, seq)), fresh nodes via with_derived_id), and its precondition is enforced: merge_cmd_preliminary_checks calls check_genome_names before renumbering.
• N3. renumber_paths correctly drops the old entity-count invariant check, since the path-map is injective by rank and only path ids move.
• N4. No concurrency defects: every new id is a pure fixed-seed hash of the entity's own contents stored in a BTreeMap, so the touched code is schedule-independent; the one parallel-execution change removes a reproducibility bug (F12) rather than adding one.
• N5. New tests are rstest functions with distinct names, each asserting one behavior with a stated oracle in its doc comment.