From 348e51d215b44d0de72566d7dcfd4b42767ac667 Mon Sep 17 00:00:00 2001 From: Marco Molari Date: Mon, 17 Aug 2026 11:52:25 +0200 Subject: [PATCH 1/6] refactor(pangraph): derive block and node ids from genome names --- CHANGELOG.md | 2 + docs/docs/reference.md | 4 +- .../minimap2_lib/align_with_minimap2_lib.rs | 7 +- .../pangraph/src/circularize/merge_blocks.rs | 4 +- .../pangraph/src/commands/merge/merge_args.rs | 11 +- .../pangraph/src/commands/merge/merge_run.rs | 16 +- .../src/commands/simplify/simplify_run.rs | 7 +- packages/pangraph/src/pangraph/pangraph.rs | 338 +++++++++--------- .../pangraph/src/pangraph/pangraph_block.rs | 26 +- .../pangraph/src/pangraph/pangraph_node.rs | 17 +- .../pangraph/src/pangraph/pangraph_path.rs | 15 + packages/pangraph/src/pangraph/slice.rs | 7 +- packages/pangraph/tests/itest_merge.rs | 118 +++++- 13 files changed, 348 insertions(+), 224 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 107a9f35..84a8275f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,8 @@ - added `pangraph merge` command, to combine two existing pangenome graphs into a single one, see #197. - `pangraph build` now accepts a single input sequence, and rejects inputs with duplicate genome names. - more strict checks on input sequences ids: duplicated or empty ids are now rejected. The `--verify` options compare the input sequence by id and not by order. +- block and node identifiers are now derived from genome names instead of the order the input sequences are read in, enforcing order-independence. +- fixed `pangraph build` producing a different graph on repeated runs over the same input: alignment hits were collected in thread completion order, which decided how equally scoring merges were broken apart. ## 1.3.0 diff --git a/docs/docs/reference.md b/docs/docs/reference.md index e9572c6b..6404ff72 100644 --- a/docs/docs/reference.md +++ b/docs/docs/reference.md @@ -153,12 +153,12 @@ Merge two pangenome graphs into a single one * `` — Path to the first input graph, in pangraph JSON format. - This graph is treated as the base: its block, node and path identifiers are preserved in the output, while those of the second graph are renumbered. When extending an existing graph with new genomes, pass the existing graph here. + This graph is treated as the base: its identifiers are preserved in the output, and the path identifiers of the second graph are renumbered to follow them. When extending an existing graph with new genomes, pass the existing graph here. Accepts plain or compressed files. Supported compression formats: `gz`, `bz2`, `xz`, `zstd`. The decompressor is chosen based on the file extension. * `` — Path to the second input graph, in pangraph JSON format. - Its identifiers are renumbered so that they do not clash with those of the first graph. Its genomes appear after those of the first graph in the output. + Its path identifiers are renumbered to follow those of the first graph, so its genomes appear after them in the output. Block and node identifiers are left alone: they are derived from genome names, which must be distinct across the two graphs, so they cannot clash. ###### **Options:** diff --git a/packages/pangraph/src/align/minimap2_lib/align_with_minimap2_lib.rs b/packages/pangraph/src/align/minimap2_lib/align_with_minimap2_lib.rs index 348e4fb9..feab7d53 100644 --- a/packages/pangraph/src/align/minimap2_lib/align_with_minimap2_lib.rs +++ b/packages/pangraph/src/align/minimap2_lib/align_with_minimap2_lib.rs @@ -61,8 +61,13 @@ fn align_with_minimap2_lib_impl( let idx = Minimap2Index::new(&seqs, &names, &args)?; + // Collected into a `Vec` first so that the parallel iterator is *indexed*: `par_bridge` hands + // work out in whatever order threads ask for it and collects in that order, which made the hit + // list vary between runs. `filter_matches` then broke equal-energy ties differently, so the same + // input could build different graphs. `Vec::into_par_iter` restores input order. let results: Vec = izip!(&seqs, &names) - .par_bridge() + .collect_vec() + .into_par_iter() .map_init( || Minimap2Mapper::new(&idx).unwrap(), move |mapper, (seq, name)| { diff --git a/packages/pangraph/src/circularize/merge_blocks.rs b/packages/pangraph/src/circularize/merge_blocks.rs index 7035fa5d..85bf76ee 100644 --- a/packages/pangraph/src/circularize/merge_blocks.rs +++ b/packages/pangraph/src/circularize/merge_blocks.rs @@ -51,7 +51,7 @@ fn orient_merging_edge(graph: &Pangraph, edge: &Edge) -> Edge { fn find_node_pairings(graph: &Pangraph, edge: &Edge) -> (BTreeMap, BTreeMap) { let mut node_pairings = btreemap! {}; let mut new_nodes = btreemap! {}; - for (&path_id, path) in &graph.paths { + for path in graph.paths.values() { let n = path.nodes.len(); let i = if path.circular { n } else { n - 1 }; for idx in 0..i { @@ -79,7 +79,7 @@ fn find_node_pairings(graph: &Pangraph, edge: &Edge) -> (BTreeMap Result<(), Report> { merge_cmd_preliminary_checks(args, &left, &right).wrap_err("When performing preliminary checks before merging")?; - // The two graphs were built independently, so their identifiers almost certainly collide. - // Namespace the second graph before joining them. + // Block and node ids are derived from genome names, which the checks above established are + // distinct across the two graphs, so they cannot collide. Path ids are sequential within each + // graph, so the appended graph is lifted above the first one. let right = right - .make_disjoint_from(&left) - .wrap_err("When making the identifiers of the two input graphs disjoint")?; + .renumber_paths(left.path_id_upper_bound()) + .wrap_err("When renumbering the path ids of the second input graph")?; + + // Cheap, and the alternative is `graph_join` panicking on the conflicting key. + if !right.is_id_disjoint_from(&left) { + return make_error!( + "The two input graphs share block or node identifiers, so they cannot be joined. Identifiers are derived from genome names since version 1.4; graphs written by earlier versions derive them from the order of the input sequences instead, and two such graphs collide. Rebuild the input graphs with the current version of pangraph." + ); + } info!( "=== Graph merging start: graph sizes {} + {}", diff --git a/packages/pangraph/src/commands/simplify/simplify_run.rs b/packages/pangraph/src/commands/simplify/simplify_run.rs index 817ae420..f38d5a17 100644 --- a/packages/pangraph/src/commands/simplify/simplify_run.rs +++ b/packages/pangraph/src/commands/simplify/simplify_run.rs @@ -54,8 +54,11 @@ mod tests { use maplit::{btreemap, btreeset}; use pretty_assertions::assert_eq; - const NID11: NodeId = NodeId(13172209629052542373); - const NID12: NodeId = NodeId(16864511183100055928); + /// Ids of the two nodes that concatenating the blocks of each path produces, as minted by + /// [`PangraphNode::with_derived_id`]. They are hashes of the new block id, the genome's seed and + /// the node's strand and position, so renaming `pathA`/`pathB` below changes them. + const NID11: NodeId = NodeId(7517044205545980976); + const NID12: NodeId = NodeId(8586529204744949647); fn block_a() -> PangraphBlock { // 0 1 2 3 diff --git a/packages/pangraph/src/pangraph/pangraph.rs b/packages/pangraph/src/pangraph/pangraph.rs index df0a6fe3..ec41c687 100644 --- a/packages/pangraph/src/pangraph/pangraph.rs +++ b/packages/pangraph/src/pangraph/pangraph.rs @@ -10,9 +10,8 @@ use crate::representation::seq::Seq; use crate::tree::clade::WithNewickName; use crate::utils::id::id; use crate::utils::map_merge::{ConflictResolution, map_merge}; -use crate::{make_internal_error, make_internal_report, make_report}; +use crate::{make_internal_report, make_report}; use eyre::{Report, WrapErr}; -use log::warn; use maplit::btreemap; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; @@ -30,8 +29,16 @@ pub struct Pangraph { impl Pangraph { pub fn singleton(fasta: FastaRecord, strand: Strand, circular: bool) -> Self { let tot_len = fasta.seq.len(); - let node_id = NodeId(fasta.index); - let block_id = BlockId(fasta.index); + // Block and node ids are seeded from the genome name rather than from the record index, so that + // two graphs built independently cannot collide: names are unique within a build, and `merge` + // rejects graphs that share one. Seeding from the index instead made every build start at + // `0, 1, 2, ...`, so merging two graphs required relabeling one of them first. + // + // Path ids stay sequential: they double as the ordering index of the genomes, and keying the + // `paths` map by a hash would scramble genome order in every output. + let seed = id(&fasta.seq_name); + let node_id = NodeId(seed); + let block_id = BlockId(seed); let block = PangraphBlock::from_consensus(fasta.seq, block_id, node_id); let path_id = PathId(fasta.index); let node_position = if circular { (0, 0) } else { (0, tot_len) }; // path wraps around if circular @@ -67,50 +74,35 @@ impl Pangraph { self.blocks.values().map(|block| block.consensus()) } - /// Returns this graph with every block, node and path id re-derived. + /// Returns this graph with its path ids renumbered contiguously from `offset`, preserving their + /// relative order. /// - /// Block and node ids become `id((salt, old_id))`; path ids are renumbered contiguously from - /// `path_id_offset`, preserving their relative order (path ids double as the ordering index of - /// the genomes, so they are kept small and sequential rather than hashed). + /// Blocks are untouched and node ids are preserved: only the `path_id` a node stores, and the id + /// of the path itself, are rewritten. Node ids are derived from [`PangraphPath::seed`] rather + /// than from the path id (see [`PangraphNode::with_derived_id`]), so renumbering cannot put a + /// node id out of step with the contents it was derived from. /// - /// Consensuses, edits, names, descriptions, strands and positions are moved over untouched: the - /// relabeled graph describes exactly the same sequences as the original one. The graph is taken - /// by value so that the sequences can be moved rather than copied. - pub fn relabel(self, salt: usize, path_id_offset: usize) -> Result { + /// Used by `merge` to lift one graph's genomes above the other's: path ids are sequential within + /// each graph, so two graphs built independently always collide on them. The graph is taken by + /// value so that the sequences can be moved rather than copied. + /// + /// Errors if a node refers to a path the graph does not contain, which can only happen in a graph + /// that pangraph did not write. + pub fn renumber_paths(self, offset: usize) -> Result { let Self { paths, blocks, nodes } = self; - let (n_blocks, n_nodes, n_paths) = (blocks.len(), nodes.len(), paths.len()); - - let block_map: BTreeMap = blocks.keys().map(|&bid| (bid, BlockId(id((salt, bid))))).collect(); - - let node_map: BTreeMap = nodes.keys().map(|&nid| (nid, NodeId(id((salt, nid))))).collect(); let path_map: BTreeMap = paths .keys() .enumerate() - .map(|(rank, &pid)| (pid, PathId(path_id_offset + rank))) + .map(|(rank, &pid)| (pid, PathId(offset + rank))) .collect(); - // The three maps above are keyed by the *map keys* of this graph, so looking an entity's own new - // id up by its key cannot miss. Ids taken from an entity's *contents* are a different matter: - // they can dangle, or disagree with the key they are stored under, in a graph that pangraph did - // not write, so those lookups are fallible. - let blocks: BTreeMap = blocks - .into_iter() - .map(|(bid, block)| { - let block = block.relabel(block_map[&bid], &node_map)?; - Ok((block.id(), block)) - }) - .collect::>()?; - + // `path_map` is keyed by the map keys of this graph, so looking a path's own new id up by its + // key cannot miss. The path id read off a node's *contents* is a different matter: it can + // dangle in a graph that pangraph did not write, so that lookup is fallible. let nodes: BTreeMap = nodes .into_iter() .map(|(nid, node)| { - let block_id = *block_map.get(&node.block_id()).ok_or_else(|| { - make_report!( - "Node {nid} refers to block {}, which the graph does not contain", - node.block_id() - ) - })?; let path_id = *path_map.get(&node.path_id()).ok_or_else(|| { make_report!( "Node {nid} refers to path {}, which the graph does not contain", @@ -118,8 +110,8 @@ impl Pangraph { ) })?; - let node = PangraphNode::new(Some(node_map[&nid]), block_id, path_id, node.strand(), node.position()); - Ok((node.id(), node)) + let node = PangraphNode::new(Some(nid), node.block_id(), path_id, node.strand(), node.position()); + Ok((nid, node)) }) .collect::>()?; @@ -127,73 +119,32 @@ impl Pangraph { .into_iter() .map(|(pid, mut path)| { path.id = path_map[&pid]; - for nid in &mut path.nodes { - let old = *nid; - *nid = *node_map - .get(&old) - .ok_or_else(|| make_report!("Path {pid} refers to node {old}, which the graph does not contain"))?; - } - Ok((path.id, path)) + (path.id, path) }) - .collect::>()?; - - // An injective relabeling cannot change the number of entities. If it did, two distinct ids - // were mapped onto the same one and entities were silently dropped. - if (blocks.len(), nodes.len(), paths.len()) != (n_blocks, n_nodes, n_paths) { - return make_internal_error!( - "When relabeling graph ids: expected {n_blocks} blocks, {n_nodes} nodes and {n_paths} paths, but got {} blocks, {} nodes and {} paths", - blocks.len(), - nodes.len(), - paths.len(), - ); - } + .collect(); Ok(Self { paths, blocks, nodes }) } /// Returns true if this graph shares no block, node or path id with `other`. + /// + /// Block and node ids are derived from genome names, which `merge` requires to be distinct across + /// the two graphs, so this holds by construction for graphs written by pangraph 1.4 or later. + /// Graphs written by earlier versions derive their ids from the input order instead, and two of + /// those do collide, which is what `merge` uses this to detect. pub fn is_id_disjoint_from(&self, other: &Self) -> bool { self.blocks.keys().all(|bid| !other.blocks.contains_key(bid)) && self.nodes.keys().all(|nid| !other.nodes.contains_key(nid)) && self.paths.keys().all(|pid| !other.paths.contains_key(pid)) } - /// Relabels this graph so that it shares no identifier with `other`, which is left untouched. + /// Returns the smallest path id that is above every path id of this graph. /// - /// Two graphs built independently always collide: `Pangraph::singleton` labels the first genome - /// of every build with path, block and node id `0`, and blocks that never merge keep that id all - /// the way to the final graph. Merging therefore requires namespacing one of the two graphs - /// first. - pub fn make_disjoint_from(self, other: &Self) -> Result { - const MAX_ATTEMPTS: usize = 8; - - // Path ids are assigned above every path id of `other`, so they cannot collide by construction. - let path_id_offset = other.paths.keys().map(|pid| pid.0 + 1).max().unwrap_or(0); - - // The salt must differ from the one used by any earlier merge whose relabeled ids survive into - // `other`, otherwise those ids get re-derived a second time and land on themselves. The path id - // offset provides that: a merger salted with `offset(P)` produces a graph with at least one more - // genome than `P`, so a later merge with that graph on the left uses a strictly larger offset. - let mut relabeled = self; - for attempt in 0..MAX_ATTEMPTS { - relabeled = relabeled.relabel(id((path_id_offset, attempt)), path_id_offset)?; - - if relabeled.is_id_disjoint_from(other) { - return Ok(relabeled); - } - - // Relabeling is a composition of injective maps, so retrying on top of the previous attempt is - // safe, and path ids are assigned by rank rather than derived from the previous id, so they do - // not drift. Reaching this point needs either a genuine hash collision, or an operation that - // breaks the monotonicity of the offset: `simplify` drops paths without renumbering, so - // `build -> merge -> merge -> simplify -> merge` can bring the offset back to a value already - // used as a salt. - warn!("Identifier collision when relabeling graph ids (attempt {attempt}); retrying"); - } - - make_internal_error!( - "When making graphs id-disjoint: no collision-free relabeling of block and node ids found after {MAX_ATTEMPTS} attempts" - ) + /// The offset `merge` renumbers the appended graph from, so that the two sets of path ids cannot + /// overlap. Not simply the number of paths: `simplify` drops paths without renumbering, so path + /// ids are not necessarily contiguous. + pub fn path_id_upper_bound(&self) -> usize { + self.paths.keys().map(|pid| pid.0 + 1).max().unwrap_or(0) } pub fn update(&mut self, u: &GraphUpdate) { @@ -615,77 +566,51 @@ mod tests { assert_eq!(g.newick_name(), expected); } - /// Builds a two-genome graph whose ids are the small sequential integers that `build` assigns to - /// blocks that never merge. Two such graphs collide on every single id. - fn colliding_graph(names: [&str; 2]) -> Pangraph { + /// Builds a two-genome graph the way `build` would, with block and node ids seeded from the + /// genome names. Two such graphs collide only on their path ids. + fn two_genome_graph(names: [&str; 2]) -> Pangraph { + let seeds = names.map(|name| id(name.to_owned())); + let (b0, b1) = (BlockId(seeds[0]), BlockId(seeds[1])); + let (n0, n1) = (NodeId(seeds[0]), NodeId(seeds[1])); + let blocks = btreemap! { - BlockId(0) => PangraphBlock::new(BlockId(0), "ACGTACGT", btreemap!{ NodeId(0) => Edit::empty() }), - BlockId(1) => PangraphBlock::new(BlockId(1), "TTTTGGGG", btreemap!{ NodeId(1) => Edit::empty() }), + b0 => PangraphBlock::new(b0, "ACGTACGT", btreemap!{ n0 => Edit::empty() }), + b1 => PangraphBlock::new(b1, "TTTTGGGG", btreemap!{ n1 => Edit::empty() }), }; let nodes = btreemap! { - NodeId(0) => PangraphNode::new(Some(NodeId(0)), BlockId(0), PathId(0), Forward, (0, 8)), - NodeId(1) => PangraphNode::new(Some(NodeId(1)), BlockId(1), PathId(1), Reverse, (0, 8)), + n0 => PangraphNode::new(Some(n0), b0, PathId(0), Forward, (0, 8)), + n1 => PangraphNode::new(Some(n1), b1, PathId(1), Reverse, (0, 8)), }; let paths = btreemap! { - PathId(0) => PangraphPath::new(Some(PathId(0)), [NodeId(0)], 8, false, Some(names[0].to_owned()), None), - PathId(1) => PangraphPath::new(Some(PathId(1)), [NodeId(1)], 8, false, Some(names[1].to_owned()), None), + PathId(0) => PangraphPath::new(Some(PathId(0)), [n0], 8, false, Some(names[0].to_owned()), None), + PathId(1) => PangraphPath::new(Some(PathId(1)), [n1], 8, false, Some(names[1].to_owned()), None), }; Pangraph { paths, blocks, nodes } } /// A graph read from JSON is keyed by the object key, while each entity also stores its own id; - /// serde never checks that the two agree. `relabel` resolves entities by key, so a disagreement - /// used to abort the process with a bare "no entry found for key" panic and no indication of - /// which file was at fault. It must be a reportable error instead. - #[rstest] - fn test_relabel_reports_block_stored_under_a_mismatched_id() { - let mut graph = colliding_graph(["a", "b"]); - let block = graph.blocks.remove(&BlockId(0)).unwrap(); - graph.blocks.insert(BlockId(42), block); // key 42, but the block still reports id 0 - - let err = report_to_string(&graph.clone().relabel(1, 7).unwrap_err()); - assert!(err.contains("does not contain"), "unexpected error: {err}"); - - // `sanity_check` never compared key against stored id, so it used to pass this graph through. - let err = report_to_string(&graph.sanity_check().unwrap_err()); - assert!(err.contains("stored under id 42"), "unexpected error: {err}"); - assert!(err.contains("reports id 0"), "unexpected error: {err}"); - } - - #[rstest] - fn test_relabel_reports_path_referring_to_a_missing_node() { - let mut graph = colliding_graph(["a", "b"]); - graph.paths.get_mut(&PathId(0)).unwrap().nodes = vec![NodeId(99)]; - - let err = report_to_string(&graph.relabel(1, 7).unwrap_err()); - assert!( - err.contains("Path 0 refers to node 99, which the graph does not contain"), - "unexpected error: {err}" - ); - } - + /// serde never checks that the two agree. `renumber_paths` resolves paths by key, so a + /// disagreement would otherwise abort the process with a bare "no entry found for key" panic and + /// no indication of which file was at fault. #[rstest] - fn test_relabel_reports_node_referring_to_a_missing_block() { - let mut graph = colliding_graph(["a", "b"]); - let node = graph.nodes.get_mut(&NodeId(0)).unwrap(); - *node = PangraphNode::new( - Some(NodeId(0)), - BlockId(99), - node.path_id(), - node.strand(), - node.position(), - ); - - let err = report_to_string(&graph.relabel(1, 7).unwrap_err()); + fn test_renumber_paths_reports_node_referring_to_a_missing_path() { + let mut graph = two_genome_graph(["a", "b"]); + let nid = graph.node_ids().next().unwrap(); + let node = &graph.nodes[&nid]; + let detached = PangraphNode::new(Some(nid), node.block_id(), PathId(99), node.strand(), node.position()); + graph.nodes.insert(nid, detached); + + let err = report_to_string(&graph.renumber_paths(7).unwrap_err()); assert!( - err.contains("Node 0 refers to block 99, which the graph does not contain"), + err.contains("refers to path 99, which the graph does not contain"), "unexpected error: {err}" ); } #[rstest] - fn test_relabel_keeps_graph_consistent() { - let graph = colliding_graph(["a", "b"]).relabel(1, 7).unwrap(); + fn test_renumber_paths_keeps_graph_consistent() { + let original = two_genome_graph(["a", "b"]); + let graph = original.clone().renumber_paths(7).unwrap(); graph.sanity_check().unwrap(); assert_eq!(graph.blocks.len(), 2); @@ -699,15 +624,20 @@ mod tests { vec![Some(o!("a")), Some(o!("b"))] ); - // block and node ids are re-derived, and no longer the original small integers - assert!(graph.block_ids().all(|bid| bid.0 > 1)); - assert!(graph.node_ids().all(|nid| nid.0 > 1)); + // blocks and node ids are untouched: only the path id a node stores is rewritten + assert_eq!(graph.blocks, original.blocks); + assert_eq!(graph.node_ids().collect_vec(), original.node_ids().collect_vec()); + for path in graph.paths.values() { + for nid in path.nodes() { + assert_eq!(graph.nodes[nid].path_id(), path.id()); + } + } } #[rstest] - fn test_relabel_preserves_sequences() { - let original = colliding_graph(["a", "b"]); - let relabeled = original.clone().relabel(3, 0).unwrap(); + fn test_renumber_paths_preserves_sequences() { + let original = two_genome_graph(["a", "b"]); + let renumbered = original.clone().renumber_paths(5).unwrap(); let seqs = |g: &Pangraph| { reconstruct(g) @@ -716,34 +646,47 @@ mod tests { .unwrap() }; - assert_eq!(seqs(&original), seqs(&relabeled)); + assert_eq!(seqs(&original), seqs(&renumbered)); } + /// `simplify` drops paths without renumbering the survivors, so the offset cannot be taken to be + /// the number of paths. #[rstest] - fn test_relabel_is_deterministic() { - let first = colliding_graph(["a", "b"]).relabel(2, 5).unwrap(); - let second = colliding_graph(["a", "b"]).relabel(2, 5).unwrap(); - assert_eq!(first, second); + fn test_path_id_upper_bound_clears_sparse_path_ids() { + let mut graph = two_genome_graph(["a", "b"]); + assert_eq!(graph.path_id_upper_bound(), 2); + + graph.paths.remove(&PathId(0)); + assert_eq!(graph.paths.len(), 1); + assert_eq!(graph.path_id_upper_bound(), 2); } #[rstest] - fn test_make_disjoint_from() { - let left = colliding_graph(["a", "b"]); - let right = colliding_graph(["c", "d"]); - - // the two graphs share every single id before relabeling - assert!(!right.is_id_disjoint_from(&left)); + fn test_path_id_upper_bound_of_an_empty_graph() { + let graph = Pangraph { + paths: btreemap! {}, + blocks: btreemap! {}, + nodes: btreemap! {}, + }; + assert_eq!(graph.path_id_upper_bound(), 0); + } - let right = right.make_disjoint_from(&left).unwrap(); + /// The property the whole identifier model rests on: two graphs built from differently named + /// genomes share no block or node id, so `merge` can join them without relabeling either. + #[rstest] + fn test_graphs_with_distinct_genome_names_are_id_disjoint() { + let left = two_genome_graph(["a", "b"]); + let right = two_genome_graph(["c", "d"]) + .renumber_paths(left.path_id_upper_bound()) + .unwrap(); assert!(right.is_id_disjoint_from(&left)); right.sanity_check().unwrap(); - // the left graph is untouched, and the right graph's genomes follow it assert_eq!(left.path_ids().collect_vec(), vec![PathId(0), PathId(1)]); assert_eq!(right.path_ids().collect_vec(), vec![PathId(2), PathId(3)]); - // joining the two no longer conflicts + // joining the two does not conflict let joined = crate::pangraph::graph_merging::graph_join(&left, &right); joined.sanity_check().unwrap(); assert_eq!(joined.paths.len(), 4); @@ -751,17 +694,20 @@ mod tests { assert_eq!(joined.nodes.len(), 4); } - /// Appending to a graph that already absorbed a relabeled graph. With a constant salt the ids of - /// the third graph were re-derived exactly onto those the second one left behind, so the second - /// append always failed. Every graph here carries the same small ids, which is what `build` - /// assigns to blocks and nodes that never merge. + /// Appending to a graph that already absorbed another one. This needed a varying relabeling salt + /// back when block and node ids were re-derived on merge; deriving them from genome names instead + /// makes it hold with no bookkeeping at all. #[rstest] - fn test_make_disjoint_from_after_a_previous_merge() { - let first = colliding_graph(["a", "b"]); - let second = colliding_graph(["c", "d"]).make_disjoint_from(&first).unwrap(); + fn test_appending_to_an_already_merged_graph() { + let first = two_genome_graph(["a", "b"]); + let second = two_genome_graph(["c", "d"]) + .renumber_paths(first.path_id_upper_bound()) + .unwrap(); let joined = crate::pangraph::graph_merging::graph_join(&first, &second); - let third = colliding_graph(["e", "f"]).make_disjoint_from(&joined).unwrap(); + let third = two_genome_graph(["e", "f"]) + .renumber_paths(joined.path_id_upper_bound()) + .unwrap(); assert!(third.is_id_disjoint_from(&joined)); third.sanity_check().unwrap(); @@ -774,4 +720,48 @@ mod tests { assert_eq!(joined.blocks.len(), 6); assert_eq!(joined.nodes.len(), 6); } + + /// Two graphs holding the same genome do collide, which is why `merge` rejects them by name + /// before it ever gets as far as joining them. + #[rstest] + fn test_graphs_sharing_a_genome_name_are_not_id_disjoint() { + let left = two_genome_graph(["a", "b"]); + let right = two_genome_graph(["a", "c"]) + .renumber_paths(left.path_id_upper_bound()) + .unwrap(); + + assert!(!right.is_id_disjoint_from(&left)); + } + + /// Ids no longer depend on the order the input sequences were read in, only on their names. + #[rstest] + fn test_singleton_ids_are_seeded_from_the_name_not_the_index() { + let singleton = |name: &str, index: usize| { + Pangraph::singleton( + FastaRecord { + seq_name: name.to_owned(), + desc: None, + seq: Seq::from_str("ACGTACGT"), + index, + }, + Forward, + false, + ) + }; + + // the same genome read at a different position in the input gets the same block and node ids + let first = singleton("a", 0); + let shifted = singleton("a", 7); + assert_eq!(first.block_ids().collect_vec(), shifted.block_ids().collect_vec()); + assert_eq!(first.node_ids().collect_vec(), shifted.node_ids().collect_vec()); + + // but the path id still records where it was read, so genome order survives + assert_eq!(first.path_ids().collect_vec(), vec![PathId(0)]); + assert_eq!(shifted.path_ids().collect_vec(), vec![PathId(7)]); + + // and a different genome read at the same position gets different block and node ids + let other = singleton("b", 0); + assert_ne!(first.block_ids().collect_vec(), other.block_ids().collect_vec()); + assert_ne!(first.node_ids().collect_vec(), other.node_ids().collect_vec()); + } } diff --git a/packages/pangraph/src/pangraph/pangraph_block.rs b/packages/pangraph/src/pangraph/pangraph_block.rs index 81f838ac..384bbf0e 100644 --- a/packages/pangraph/src/pangraph/pangraph_block.rs +++ b/packages/pangraph/src/pangraph/pangraph_block.rs @@ -3,6 +3,7 @@ use crate::align::map_variations::{BandParameters, map_variations}; use crate::io::fasta::FastaRecord; use crate::io::json::{JsonPretty, json_write_str}; use crate::io::seq::reverse_complement; +use crate::make_internal_error; use crate::pangraph::edits::{Del, Edit, Ins, Sub}; use crate::pangraph::pangraph::Pangraph; use crate::pangraph::pangraph_node::NodeId; @@ -10,7 +11,6 @@ use crate::pangraph::pangraph_path::PathId; use crate::representation::seq::Seq; use crate::utils::collections::has_duplicates; use crate::utils::interval::positions_to_intervals; -use crate::{make_internal_error, make_report}; use derive_more::{Display, From}; use eyre::{Report, WrapErr}; use getset::{CopyGetters, Getters}; @@ -60,30 +60,6 @@ impl PangraphBlock { } } - /// Returns this block with a new id and its alignment keys rewritten through `node_map`. - /// The consensus and the edits are moved over unchanged. - /// - /// Errors if an alignment is keyed by a node the graph does not contain, which can only happen in - /// a graph that pangraph did not write. - pub fn relabel(self, id: BlockId, node_map: &BTreeMap) -> Result { - let alignments = self - .alignments - .into_iter() - .map(|(nid, edit)| { - let new_nid = *node_map - .get(&nid) - .ok_or_else(|| make_report!("Block {id} aligns node {nid}, which the graph does not contain"))?; - Ok((new_nid, edit)) - }) - .collect::>()?; - - Ok(Self { - id, - consensus: self.consensus, - alignments, - }) - } - pub fn reverse_complement(&self) -> Result { let rev_cons = reverse_complement(&self.consensus)?; diff --git a/packages/pangraph/src/pangraph/pangraph_node.rs b/packages/pangraph/src/pangraph/pangraph_node.rs index 225a040d..91498ce9 100644 --- a/packages/pangraph/src/pangraph/pangraph_node.rs +++ b/packages/pangraph/src/pangraph/pangraph_node.rs @@ -1,5 +1,5 @@ use crate::pangraph::pangraph_block::BlockId; -use crate::pangraph::pangraph_path::PathId; +use crate::pangraph::pangraph_path::{PangraphPath, PathId}; use crate::pangraph::strand::Strand; use crate::utils::id::id; use derive_more::{Display, From}; @@ -52,6 +52,21 @@ impl PangraphNode { } } + /// Creates a node placing `block_id` on `path`, with an id derived from its contents. + /// + /// Genomes are discriminated by [`PangraphPath::seed`] rather than by their path id, so the + /// resulting id does not depend on the order in which the input sequences were read. The path id + /// is still what the node stores. + pub fn with_derived_id(block_id: BlockId, path: &PangraphPath, strand: Strand, position: (usize, usize)) -> Self { + Self { + id: id((&block_id, &path.seed(), &strand, &position)), + block_id, + path_id: path.id(), + strand, + position, + } + } + // this is almost equivalent to checking if the node is empty // except for an edge case: when a circular path contains only // one node. In this case even if the node is not empty, the diff --git a/packages/pangraph/src/pangraph/pangraph_path.rs b/packages/pangraph/src/pangraph/pangraph_path.rs index 0c19a10a..6c5e55a6 100644 --- a/packages/pangraph/src/pangraph/pangraph_path.rs +++ b/packages/pangraph/src/pangraph/pangraph_path.rs @@ -51,4 +51,19 @@ impl PangraphPath { desc, } } + + /// Order-independent identity of the genome on this path, used as the discriminator when deriving + /// node ids. + /// + /// Taken from the genome name rather than from the path id, so that node ids do not depend on the + /// order in which the input sequences were read, and do not change when `renumber_paths` shifts + /// path ids during a merge. Genome names are unique within a graph, and `merge` rejects graphs + /// that share one, so the seed identifies a genome as well as the path id does. + /// + /// Recomputed on demand rather than stored: a stored field would have to be kept out of the JSON + /// and then recomputed on load anyway, and silently defaults to the same value for every path if + /// that is forgotten. Falls back to the path id for unnamed paths, which pangraph never writes. + pub fn seed(&self) -> usize { + self.name.as_ref().map_or(self.id.0, id) + } } diff --git a/packages/pangraph/src/pangraph/slice.rs b/packages/pangraph/src/pangraph/slice.rs index 620f9458..98e8fd50 100644 --- a/packages/pangraph/src/pangraph/slice.rs +++ b/packages/pangraph/src/pangraph/slice.rs @@ -167,16 +167,17 @@ pub fn block_slice( old_strandedness }; - let path_L = G.paths[&old_node.path_id()].tot_len; + let path = &G.paths[&old_node.path_id()]; + let path_L = path.tot_len; let node_coords = interval_node_coords(i, edits, block_L); - let circular = G.paths[&old_node.path_id()].circular(); + let circular = path.circular(); let new_pos = if circular { new_position_circular(old_node.position(), node_coords, path_L, old_strandedness) } else { new_position_non_circular(old_node.position(), node_coords, old_strandedness) }; - let new_node = PangraphNode::new(None, i.new_block_id, old_node.path_id(), new_strand, new_pos); + let new_node = PangraphNode::with_derived_id(i.new_block_id, path, new_strand, new_pos); // extract edits for the slice let new_edits = slice_edits(i, edits, block_L); diff --git a/packages/pangraph/tests/itest_merge.rs b/packages/pangraph/tests/itest_merge.rs index f2c0f5f1..7b3bc9cf 100644 --- a/packages/pangraph/tests/itest_merge.rs +++ b/packages/pangraph/tests/itest_merge.rs @@ -12,7 +12,11 @@ mod tests { use pangraph::io::fasta::{FastaReader, FastaRecord}; use pangraph::io::json::{JsonPretty, json_write_file}; use pangraph::pangraph::pangraph::Pangraph; + use pangraph::pangraph::pangraph_block::{BlockId, PangraphBlock}; + use pangraph::pangraph::pangraph_node::{NodeId, PangraphNode}; + use pangraph::pangraph::pangraph_path::{PangraphPath, PathId}; use pangraph::pangraph::reconstruct::reconstruct; + use pangraph::pangraph::strand::Strand::Forward; use pangraph::representation::seq::Seq; use pangraph::utils::error::report_to_string; use pretty_assertions::assert_eq; @@ -141,11 +145,12 @@ mod tests { Ok(()) } - /// Appending to a graph that is itself the result of an earlier merge. Identifiers relabeled by - /// the first merge survive into its output whenever a block or node finds no homologue, and a - /// constant relabeling salt then re-derived the third graph's identifiers onto exactly those - /// values, so the second append always failed. The three genomes here are mutually unrelated, so - /// nothing aligns and every identifier survives; homologous appends never hit this. + /// Appending to a graph that is itself the result of an earlier merge. Identifiers minted by the + /// first merge survive into its output whenever a block or node finds no homologue, so this is + /// the case that used to need a varying relabeling salt to keep the third graph's identifiers off + /// them. Deriving identifiers from genome names removes the problem at the source. The three + /// genomes here are mutually unrelated, so nothing aligns and every identifier survives; + /// homologous appends never exercised this. #[rstest] fn itest_merge_appends_to_an_already_merged_graph() -> Result<(), Report> { let dir = tempdir()?; @@ -167,6 +172,109 @@ mod tests { Ok(()) } + /// Maps each genome name to the ids of the blocks its path walks through, in order. Comparing the + /// id *sets* of two graphs would not do: `build` used to label singleton blocks `0, 1, 2, ...` + /// whatever the genome, so the sets matched even when every genome held a different block. + fn blocks_by_genome(graph: &Pangraph) -> BTreeMap> { + graph + .paths + .values() + .map(|path| { + let blocks = path.nodes().iter().map(|nid| graph.nodes[nid].block_id()).collect_vec(); + (path.name().clone().unwrap_or_default(), blocks) + }) + .collect() + } + + /// Block and node identifiers are derived from genome names rather than from the order the input + /// sequences were read in, so the same genomes under the same guide tree produce the same graph + /// whichever order they arrive in. Only path ids, which deliberately record the input order, are + /// expected to differ. + #[rstest] + fn itest_build_ids_do_not_depend_on_input_order() -> Result<(), Report> { + let dir = tempdir()?; + let fastas = read_records("../../data/ges-1.fa", 4)?; + let names = fastas.iter().map(|f| f.seq_name.clone()).collect_vec(); + + // Pinned, so that the input order is the only thing that varies between the two builds. + let newick = dir.path().join("guide.nwk"); + std::fs::write( + &newick, + format!("(({},{}),({},{}));", names[0], names[1], names[2], names[3]), + )?; + + let build_in_order = |fastas: Vec| -> Result { + let args = PangraphBuildArgs { + circular: false, + guide_tree: Some(newick.clone()), + ..PangraphBuildArgs::default() + }; + build(fastas, &args, true) + }; + + // Reversed *and* re-indexed, which is what reading the same genomes from a reordered file does. + let mut backwards = fastas.clone(); + backwards.reverse(); + for (index, record) in backwards.iter_mut().enumerate() { + record.index = index; + } + + let forward = build_in_order(fastas)?; + let reversed = build_in_order(backwards)?; + + assert_eq!(blocks_by_genome(&forward), blocks_by_genome(&reversed)); + assert_eq!( + forward.node_ids().collect::>(), + reversed.node_ids().collect::>() + ); + assert_eq!(sequences(&forward)?, sequences(&reversed)?); + + // path ids still record the input order, so the genomes come back in the order they were read + assert_eq!(forward.path_names().flatten().collect_vec(), names); + assert_eq!( + reversed.path_names().flatten().collect_vec(), + names.iter().rev().collect_vec() + ); + + Ok(()) + } + + /// A graph written by pangraph 1.3 or earlier labels its first genome with block, node and path + /// id `0`, whatever that genome is called, so two of them collide even though their names differ. + /// `merge` has to say so: without the check, `graph_join` panics on the conflicting key. + #[rstest] + fn itest_merge_rejects_graphs_with_colliding_identifiers() -> Result<(), Report> { + let dir = tempdir()?; + + // A singleton graph in the pre-1.4 identifier scheme, where ids came from the record index. + let legacy_graph = |name: &str, seq: &str| { + let (bid, nid, pid) = (BlockId(0), NodeId(0), PathId(0)); + Pangraph { + blocks: BTreeMap::from([(bid, PangraphBlock::from_consensus(seq, bid, nid))]), + nodes: BTreeMap::from([(nid, PangraphNode::new(Some(nid), bid, pid, Forward, (0, seq.len())))]), + paths: BTreeMap::from([( + pid, + PangraphPath::new(Some(pid), [nid], seq.len(), false, Some(name.to_owned()), None), + )]), + } + }; + + let left = dir.path().join("left.json"); + let right = dir.path().join("right.json"); + json_write_file(&left, &legacy_graph("genome_a", "ACGTACGTAC"), JsonPretty(false))?; + json_write_file(&right, &legacy_graph("genome_b", "TTTTGGGGCC"), JsonPretty(false))?; + + let result = merge_run(&merge_args(left, right, dir.path().join("out.json"))); + + let error = report_to_string(&result.unwrap_err()); + assert!( + error.contains("share block or node identifiers"), + "unexpected error message: {error}" + ); + + Ok(()) + } + /// Merging a graph with itself duplicates every genome name, and must be rejected. #[rstest] fn itest_merge_rejects_duplicate_genome_names() -> Result<(), Report> { From e2a26222c319b4082a1f75b758b55c6c4a1e0922 Mon Sep 17 00:00:00 2001 From: Marco Molari Date: Mon, 17 Aug 2026 13:40:46 +0200 Subject: [PATCH 2/6] docs(cli): simplify the help text of the merge input arguments --- docs/docs/reference.md | 8 ++++---- packages/pangraph/src/commands/merge/merge_args.rs | 12 +++++------- 2 files changed, 9 insertions(+), 11 deletions(-) diff --git a/docs/docs/reference.md b/docs/docs/reference.md index 6404ff72..c13b3157 100644 --- a/docs/docs/reference.md +++ b/docs/docs/reference.md @@ -151,14 +151,14 @@ Merge two pangenome graphs into a single one ###### **Arguments:** -* `` — Path to the first input graph, in pangraph JSON format. - - This graph is treated as the base: its identifiers are preserved in the output, and the path identifiers of the second graph are renumbered to follow them. When extending an existing graph with new genomes, pass the existing graph here. +* `` — Path to the first input graph, in pangraph JSON format. This graph is treated as the base. Accepts plain or compressed files. Supported compression formats: `gz`, `bz2`, `xz`, `zstd`. The decompressor is chosen based on the file extension. * `` — Path to the second input graph, in pangraph JSON format. - Its path identifiers are renumbered to follow those of the first graph, so its genomes appear after them in the output. Block and node identifiers are left alone: they are derived from genome names, which must be distinct across the two graphs, so they cannot clash. + Its path identifiers are renumbered to follow those of the first graph, so its genomes appear after them in the output. + + Accepts plain or compressed files. Supported compression formats: `gz`, `bz2`, `xz`, `zstd`. The decompressor is chosen based on the file extension. ###### **Options:** diff --git a/packages/pangraph/src/commands/merge/merge_args.rs b/packages/pangraph/src/commands/merge/merge_args.rs index 6094c50f..a77e6175 100644 --- a/packages/pangraph/src/commands/merge/merge_args.rs +++ b/packages/pangraph/src/commands/merge/merge_args.rs @@ -7,11 +7,7 @@ use std::path::PathBuf; /// Merge two pangenome graphs into a single one #[derive(Parser, Debug, SmartDefault)] pub struct PangraphMergeArgs { - /// Path to the first input graph, in pangraph JSON format. - /// - /// This graph is treated as the base: its identifiers are preserved in the output, and the path - /// identifiers of the second graph are renumbered to follow them. When extending an existing - /// graph with new genomes, pass the existing graph here. + /// Path to the first input graph, in pangraph JSON format. This graph is treated as the base. /// /// Accepts plain or compressed files. Supported compression formats: `gz`, `bz2`, `xz`, `zstd`. /// The decompressor is chosen based on the file extension. @@ -22,8 +18,10 @@ pub struct PangraphMergeArgs { /// Path to the second input graph, in pangraph JSON format. /// /// Its path identifiers are renumbered to follow those of the first graph, so its genomes appear - /// after them in the output. Block and node identifiers are left alone: they are derived from - /// genome names, which must be distinct across the two graphs, so they cannot clash. + /// after them in the output. + /// + /// Accepts plain or compressed files. Supported compression formats: `gz`, `bz2`, `xz`, `zstd`. + /// The decompressor is chosen based on the file extension. #[clap(value_hint = ValueHint::FilePath)] #[clap(display_order = 2)] pub right_graph: PathBuf, From ea1f9faaa93328df23c2718e788c267bb0a7a2d1 Mon Sep 17 00:00:00 2001 From: Marco Molari Date: Tue, 18 Aug 2026 11:23:10 +0200 Subject: [PATCH 3/6] refactor(pangraph): require an explicit id in the node constructor --- .../pangraph/src/circularize/circularize.rs | 50 +++---- .../pangraph/src/circularize/merge_blocks.rs | 134 ++++++++++-------- .../src/commands/simplify/simplify_run.rs | 34 ++--- .../pangraph/src/pangraph/detach_unaligned.rs | 18 +-- packages/pangraph/src/pangraph/pangraph.rs | 44 +++--- .../pangraph/src/pangraph/pangraph_node.rs | 21 ++- packages/pangraph/src/pangraph/reconstruct.rs | 14 +- packages/pangraph/src/pangraph/reweave.rs | 22 +-- packages/pangraph/src/pangraph/slice.rs | 18 +-- .../pangraph/src/reconsensus/reconsensus.rs | 22 +-- .../pangraph/src/reconsensus/remove_nodes.rs | 10 +- packages/pangraph/tests/itest_merge.rs | 2 +- packages/pangraph/tests/itest_reconstruct.rs | 4 +- 13 files changed, 202 insertions(+), 191 deletions(-) diff --git a/packages/pangraph/src/circularize/circularize.rs b/packages/pangraph/src/circularize/circularize.rs index 0c501cad..bce68297 100644 --- a/packages/pangraph/src/circularize/circularize.rs +++ b/packages/pangraph/src/circularize/circularize.rs @@ -110,28 +110,28 @@ mod tests { #[rustfmt::skip] let nodes = btreemap! { - NodeId(10) => PangraphNode::new(Some(NodeId(10)), BlockId(1), PathId(0), Forward, (0, 0)), - NodeId(20) => PangraphNode::new(Some(NodeId(20)), BlockId(2), PathId(0), Forward, (0, 0)), - NodeId(30) => PangraphNode::new(Some(NodeId(30)), BlockId(3), PathId(0), Forward, (0, 0)), - NodeId(40) => PangraphNode::new(Some(NodeId(40)), BlockId(4), PathId(0), Forward, (0, 0)), - NodeId(11) => PangraphNode::new(Some(NodeId(11)), BlockId(1), PathId(1), Forward, (0, 0)), - NodeId(21) => PangraphNode::new(Some(NodeId(21)), BlockId(2), PathId(1), Reverse, (0, 0)), - NodeId(22) => PangraphNode::new(Some(NodeId(22)), BlockId(2), PathId(1), Forward, (0, 0)), - NodeId(31) => PangraphNode::new(Some(NodeId(31)), BlockId(3), PathId(1), Forward, (0, 0)), - NodeId(41) => PangraphNode::new(Some(NodeId(41)), BlockId(4), PathId(1), Forward, (0, 0)), - NodeId(12) => PangraphNode::new(Some(NodeId(12)), BlockId(1), PathId(2), Forward, (0, 0)), - NodeId(23) => PangraphNode::new(Some(NodeId(23)), BlockId(2), PathId(2), Forward, (0, 0)), - NodeId(32) => PangraphNode::new(Some(NodeId(32)), BlockId(3), PathId(2), Reverse, (0, 0)), - NodeId(42) => PangraphNode::new(Some(NodeId(42)), BlockId(4), PathId(2), Forward, (0, 0)), - NodeId(13) => PangraphNode::new(Some(NodeId(13)), BlockId(1), PathId(3), Forward, (0, 0)), - NodeId(33) => PangraphNode::new(Some(NodeId(33)), BlockId(3), PathId(3), Reverse, (0, 0)), - NodeId(24) => PangraphNode::new(Some(NodeId(24)), BlockId(2), PathId(3), Forward, (0, 0)), - NodeId(34) => PangraphNode::new(Some(NodeId(34)), BlockId(3), PathId(3), Reverse, (0, 0)), - NodeId(43) => PangraphNode::new(Some(NodeId(43)), BlockId(4), PathId(3), Forward, (0, 0)), - NodeId(44) => PangraphNode::new(Some(NodeId(44)), BlockId(4), PathId(4), Reverse, (0, 0)), - NodeId(35) => PangraphNode::new(Some(NodeId(35)), BlockId(3), PathId(4), Reverse, (0, 0)), - NodeId(25) => PangraphNode::new(Some(NodeId(25)), BlockId(2), PathId(4), Reverse, (0, 0)), - NodeId(14) => PangraphNode::new(Some(NodeId(14)), BlockId(1), PathId(4), Reverse, (0, 0)), + NodeId(10) => PangraphNode::new(NodeId(10), BlockId(1), PathId(0), Forward, (0, 0)), + NodeId(20) => PangraphNode::new(NodeId(20), BlockId(2), PathId(0), Forward, (0, 0)), + NodeId(30) => PangraphNode::new(NodeId(30), BlockId(3), PathId(0), Forward, (0, 0)), + NodeId(40) => PangraphNode::new(NodeId(40), BlockId(4), PathId(0), Forward, (0, 0)), + NodeId(11) => PangraphNode::new(NodeId(11), BlockId(1), PathId(1), Forward, (0, 0)), + NodeId(21) => PangraphNode::new(NodeId(21), BlockId(2), PathId(1), Reverse, (0, 0)), + NodeId(22) => PangraphNode::new(NodeId(22), BlockId(2), PathId(1), Forward, (0, 0)), + NodeId(31) => PangraphNode::new(NodeId(31), BlockId(3), PathId(1), Forward, (0, 0)), + NodeId(41) => PangraphNode::new(NodeId(41), BlockId(4), PathId(1), Forward, (0, 0)), + NodeId(12) => PangraphNode::new(NodeId(12), BlockId(1), PathId(2), Forward, (0, 0)), + NodeId(23) => PangraphNode::new(NodeId(23), BlockId(2), PathId(2), Forward, (0, 0)), + NodeId(32) => PangraphNode::new(NodeId(32), BlockId(3), PathId(2), Reverse, (0, 0)), + NodeId(42) => PangraphNode::new(NodeId(42), BlockId(4), PathId(2), Forward, (0, 0)), + NodeId(13) => PangraphNode::new(NodeId(13), BlockId(1), PathId(3), Forward, (0, 0)), + NodeId(33) => PangraphNode::new(NodeId(33), BlockId(3), PathId(3), Reverse, (0, 0)), + NodeId(24) => PangraphNode::new(NodeId(24), BlockId(2), PathId(3), Forward, (0, 0)), + NodeId(34) => PangraphNode::new(NodeId(34), BlockId(3), PathId(3), Reverse, (0, 0)), + NodeId(43) => PangraphNode::new(NodeId(43), BlockId(4), PathId(3), Forward, (0, 0)), + NodeId(44) => PangraphNode::new(NodeId(44), BlockId(4), PathId(4), Reverse, (0, 0)), + NodeId(35) => PangraphNode::new(NodeId(35), BlockId(3), PathId(4), Reverse, (0, 0)), + NodeId(25) => PangraphNode::new(NodeId(25), BlockId(2), PathId(4), Reverse, (0, 0)), + NodeId(14) => PangraphNode::new(NodeId(14), BlockId(1), PathId(4), Reverse, (0, 0)), }; #[rustfmt::skip] @@ -216,9 +216,9 @@ mod tests { BlockId(1) => block_1() }; let nodes = btreemap! { - NodeId(1) => PangraphNode::new(Some(NodeId(1)), BlockId(1), PathId(1), Forward, (0, 32)), - NodeId(2) => PangraphNode::new(Some(NodeId(2)), BlockId(1), PathId(2), Forward, (0, 31)), - NodeId(3) => PangraphNode::new(Some(NodeId(3)), BlockId(1), PathId(3), Reverse, (0, 35)) + NodeId(1) => PangraphNode::new(NodeId(1), BlockId(1), PathId(1), Forward, (0, 32)), + NodeId(2) => PangraphNode::new(NodeId(2), BlockId(1), PathId(2), Forward, (0, 31)), + NodeId(3) => PangraphNode::new(NodeId(3), BlockId(1), PathId(3), Reverse, (0, 35)) }; Pangraph { paths, blocks, nodes } } diff --git a/packages/pangraph/src/circularize/merge_blocks.rs b/packages/pangraph/src/circularize/merge_blocks.rs index 85bf76ee..b5a5ccd9 100644 --- a/packages/pangraph/src/circularize/merge_blocks.rs +++ b/packages/pangraph/src/circularize/merge_blocks.rs @@ -240,7 +240,7 @@ mod tests { use crate::circularize::circularize::remove_transitive_edges; use crate::pangraph::edits::{Del, Edit, Ins, Sub}; use crate::pangraph::pangraph_path::{PangraphPath, PathId}; - use crate::pangraph::strand::Strand::{Forward, Reverse}; + use crate::pangraph::strand::Strand::{self, Forward, Reverse}; use itertools::Itertools; use maplit::btreemap; use pretty_assertions::assert_eq; @@ -349,14 +349,14 @@ mod tests { }; let nodes = btreemap! { - NodeId(1) => PangraphNode::new(Some(NodeId(1)), BlockId(1), PathId(1), Forward, (0, 32)), - NodeId(2) => PangraphNode::new(Some(NodeId(2)), BlockId(1), PathId(2), Forward, (10, 41)), - NodeId(3) => PangraphNode::new(Some(NodeId(3)), BlockId(1), PathId(3), Reverse, (40, 5 )), - NodeId(4) => PangraphNode::new(Some(NodeId(4)), BlockId(2), PathId(1), Reverse, (32, 61)), - NodeId(5) => PangraphNode::new(Some(NodeId(5)), BlockId(2), PathId(2), Reverse, (41, 72)), - NodeId(6) => PangraphNode::new(Some(NodeId(6)), BlockId(2), PathId(3), Forward, (5, 40)), - NodeId(7) => PangraphNode::new(Some(NodeId(7)), BlockId(3), PathId(1), Forward, (61, 0 )), - NodeId(8) => PangraphNode::new(Some(NodeId(8)), BlockId(3), PathId(2), Forward, (72, 10)), + NodeId(1) => PangraphNode::new(NodeId(1), BlockId(1), PathId(1), Forward, (0, 32)), + NodeId(2) => PangraphNode::new(NodeId(2), BlockId(1), PathId(2), Forward, (10, 41)), + NodeId(3) => PangraphNode::new(NodeId(3), BlockId(1), PathId(3), Reverse, (40, 5 )), + NodeId(4) => PangraphNode::new(NodeId(4), BlockId(2), PathId(1), Reverse, (32, 61)), + NodeId(5) => PangraphNode::new(NodeId(5), BlockId(2), PathId(2), Reverse, (41, 72)), + NodeId(6) => PangraphNode::new(NodeId(6), BlockId(2), PathId(3), Forward, (5, 40)), + NodeId(7) => PangraphNode::new(NodeId(7), BlockId(3), PathId(1), Forward, (61, 0 )), + NodeId(8) => PangraphNode::new(NodeId(8), BlockId(3), PathId(2), Forward, (72, 10)), }; Pangraph { paths, blocks, nodes } @@ -384,14 +384,14 @@ mod tests { }; let nodes = btreemap! { - NodeId(1) => PangraphNode::new(Some(NodeId(1)), BlockId(1), PathId(1), Reverse, (0, 32)), - NodeId(2) => PangraphNode::new(Some(NodeId(2)), BlockId(1), PathId(2), Reverse, (10, 41)), - NodeId(3) => PangraphNode::new(Some(NodeId(3)), BlockId(1), PathId(3), Forward, (40, 5 )), - NodeId(4) => PangraphNode::new(Some(NodeId(4)), BlockId(2), PathId(1), Forward, (32, 61)), - NodeId(5) => PangraphNode::new(Some(NodeId(5)), BlockId(2), PathId(2), Forward, (41, 72)), - NodeId(6) => PangraphNode::new(Some(NodeId(6)), BlockId(2), PathId(3), Reverse, (5, 40)), - NodeId(7) => PangraphNode::new(Some(NodeId(7)), BlockId(3), PathId(1), Forward, (61, 0 )), - NodeId(8) => PangraphNode::new(Some(NodeId(8)), BlockId(3), PathId(2), Forward, (72, 10)), + NodeId(1) => PangraphNode::new(NodeId(1), BlockId(1), PathId(1), Reverse, (0, 32)), + NodeId(2) => PangraphNode::new(NodeId(2), BlockId(1), PathId(2), Reverse, (10, 41)), + NodeId(3) => PangraphNode::new(NodeId(3), BlockId(1), PathId(3), Forward, (40, 5 )), + NodeId(4) => PangraphNode::new(NodeId(4), BlockId(2), PathId(1), Forward, (32, 61)), + NodeId(5) => PangraphNode::new(NodeId(5), BlockId(2), PathId(2), Forward, (41, 72)), + NodeId(6) => PangraphNode::new(NodeId(6), BlockId(2), PathId(3), Reverse, (5, 40)), + NodeId(7) => PangraphNode::new(NodeId(7), BlockId(3), PathId(1), Forward, (61, 0 )), + NodeId(8) => PangraphNode::new(NodeId(8), BlockId(3), PathId(2), Forward, (72, 10)), }; Pangraph { paths, blocks, nodes } @@ -419,14 +419,14 @@ mod tests { }; let nodes = btreemap! { - NodeId(1) => PangraphNode::new(Some(NodeId(1)), BlockId(1), PathId(1), Forward, (0, 32)), - NodeId(2) => PangraphNode::new(Some(NodeId(2)), BlockId(1), PathId(2), Forward, (10, 41)), - NodeId(3) => PangraphNode::new(Some(NodeId(3)), BlockId(1), PathId(3), Reverse, (40, 5 )), - NodeId(4) => PangraphNode::new(Some(NodeId(4)), BlockId(2), PathId(1), Forward, (32, 61)), - NodeId(5) => PangraphNode::new(Some(NodeId(5)), BlockId(2), PathId(2), Forward, (41, 72)), - NodeId(6) => PangraphNode::new(Some(NodeId(6)), BlockId(2), PathId(3), Reverse, (5, 40)), - NodeId(7) => PangraphNode::new(Some(NodeId(7)), BlockId(3), PathId(1), Forward, (61, 0 )), - NodeId(8) => PangraphNode::new(Some(NodeId(8)), BlockId(3), PathId(2), Forward, (72, 10)), + NodeId(1) => PangraphNode::new(NodeId(1), BlockId(1), PathId(1), Forward, (0, 32)), + NodeId(2) => PangraphNode::new(NodeId(2), BlockId(1), PathId(2), Forward, (10, 41)), + NodeId(3) => PangraphNode::new(NodeId(3), BlockId(1), PathId(3), Reverse, (40, 5 )), + NodeId(4) => PangraphNode::new(NodeId(4), BlockId(2), PathId(1), Forward, (32, 61)), + NodeId(5) => PangraphNode::new(NodeId(5), BlockId(2), PathId(2), Forward, (41, 72)), + NodeId(6) => PangraphNode::new(NodeId(6), BlockId(2), PathId(3), Reverse, (5, 40)), + NodeId(7) => PangraphNode::new(NodeId(7), BlockId(3), PathId(1), Forward, (61, 0 )), + NodeId(8) => PangraphNode::new(NodeId(8), BlockId(3), PathId(2), Forward, (72, 10)), }; Pangraph { paths, blocks, nodes } @@ -505,36 +505,51 @@ mod tests { assert_eq!(block_2_revcomp(), block_2().reverse_complement().unwrap()); } + /// Builds a node the way the code under test does: with its id derived from the genome that + /// walks it, looked up in `graph`, rather than from the path id. + fn derived_node( + graph: &Pangraph, + block_id: BlockId, + path_id: PathId, + strand: Strand, + position: (usize, usize), + ) -> PangraphNode { + PangraphNode::with_derived_id(block_id, &graph.paths[&path_id], strand, position) + } + fn expected_new_nodes_a() -> BTreeMap { + let g = graph_a(); btreemap! { - NodeId(1) => PangraphNode::new(None, BlockId(1), PathId(1), Forward, (0 , 61)), - NodeId(2) => PangraphNode::new(None, BlockId(1), PathId(2), Forward, (10, 72)), - NodeId(3) => PangraphNode::new(None, BlockId(1), PathId(3), Reverse, (5, 5)), - NodeId(4) => PangraphNode::new(None, BlockId(1), PathId(1), Forward, (0 , 61)), - NodeId(5) => PangraphNode::new(None, BlockId(1), PathId(2), Forward, (10, 72)), - NodeId(6) => PangraphNode::new(None, BlockId(1), PathId(3), Reverse, (5, 5)), + NodeId(1) => derived_node(&g, BlockId(1), PathId(1), Forward, (0 , 61)), + NodeId(2) => derived_node(&g, BlockId(1), PathId(2), Forward, (10, 72)), + NodeId(3) => derived_node(&g, BlockId(1), PathId(3), Reverse, (5, 5)), + NodeId(4) => derived_node(&g, BlockId(1), PathId(1), Forward, (0 , 61)), + NodeId(5) => derived_node(&g, BlockId(1), PathId(2), Forward, (10, 72)), + NodeId(6) => derived_node(&g, BlockId(1), PathId(3), Reverse, (5, 5)), } } fn expected_new_nodes_b() -> BTreeMap { + let g = graph_b(); btreemap! { - NodeId(1) => PangraphNode::new(None, BlockId(1), PathId(1), Reverse, (0 , 61)), - NodeId(2) => PangraphNode::new(None, BlockId(1), PathId(2), Reverse, (10, 72)), - NodeId(3) => PangraphNode::new(None, BlockId(1), PathId(3), Forward, (5, 5)), - NodeId(4) => PangraphNode::new(None, BlockId(1), PathId(1), Reverse, (0 , 61)), - NodeId(5) => PangraphNode::new(None, BlockId(1), PathId(2), Reverse, (10, 72)), - NodeId(6) => PangraphNode::new(None, BlockId(1), PathId(3), Forward, (5, 5)), + NodeId(1) => derived_node(&g, BlockId(1), PathId(1), Reverse, (0 , 61)), + NodeId(2) => derived_node(&g, BlockId(1), PathId(2), Reverse, (10, 72)), + NodeId(3) => derived_node(&g, BlockId(1), PathId(3), Forward, (5, 5)), + NodeId(4) => derived_node(&g, BlockId(1), PathId(1), Reverse, (0 , 61)), + NodeId(5) => derived_node(&g, BlockId(1), PathId(2), Reverse, (10, 72)), + NodeId(6) => derived_node(&g, BlockId(1), PathId(3), Forward, (5, 5)), } } fn expected_new_nodes_c() -> BTreeMap { + let g = graph_c(); btreemap! { - NodeId(1) => PangraphNode::new(None, BlockId(1), PathId(1), Forward, (0 , 61)), - NodeId(2) => PangraphNode::new(None, BlockId(1), PathId(2), Forward, (10, 72)), - NodeId(3) => PangraphNode::new(None, BlockId(1), PathId(3), Reverse, (5, 5)), - NodeId(4) => PangraphNode::new(None, BlockId(1), PathId(1), Forward, (0 , 61)), - NodeId(5) => PangraphNode::new(None, BlockId(1), PathId(2), Forward, (10, 72)), - NodeId(6) => PangraphNode::new(None, BlockId(1), PathId(3), Reverse, (5, 5)), + NodeId(1) => derived_node(&g, BlockId(1), PathId(1), Forward, (0 , 61)), + NodeId(2) => derived_node(&g, BlockId(1), PathId(2), Forward, (10, 72)), + NodeId(3) => derived_node(&g, BlockId(1), PathId(3), Reverse, (5, 5)), + NodeId(4) => derived_node(&g, BlockId(1), PathId(1), Forward, (0 , 61)), + NodeId(5) => derived_node(&g, BlockId(1), PathId(2), Forward, (10, 72)), + NodeId(6) => derived_node(&g, BlockId(1), PathId(3), Reverse, (5, 5)), } } @@ -672,6 +687,7 @@ mod tests { // (40|-----------|40) // p3) (b1-|-----------|n3) l=67 + let g = graph_a(); let new_ids = expected_new_node_ids_a(); let blocks = btreemap! { @@ -683,11 +699,11 @@ mod tests { }; let nodes = btreemap! { - new_ids[&NodeId(1)] => PangraphNode::new(None, BlockId(1), PathId(1), Forward, (0 , 61)), - new_ids[&NodeId(2)] => PangraphNode::new(None, BlockId(1), PathId(2), Forward, (10, 72)), - new_ids[&NodeId(3)] => PangraphNode::new(None, BlockId(1), PathId(3), Reverse, (5, 5)), - NodeId(7) => PangraphNode::new(Some(NodeId(7)), BlockId(3), PathId(1), Forward, (61, 0 )), - NodeId(8) => PangraphNode::new(Some(NodeId(8)), BlockId(3), PathId(2), Forward, (72, 10)), + new_ids[&NodeId(1)] => derived_node(&g, BlockId(1), PathId(1), Forward, (0 , 61)), + new_ids[&NodeId(2)] => derived_node(&g, BlockId(1), PathId(2), Forward, (10, 72)), + new_ids[&NodeId(3)] => derived_node(&g, BlockId(1), PathId(3), Reverse, (5, 5)), + NodeId(7) => PangraphNode::new(NodeId(7), BlockId(3), PathId(1), Forward, (61, 0 )), + NodeId(8) => PangraphNode::new(NodeId(8), BlockId(3), PathId(2), Forward, (72, 10)), }; #[rustfmt::skip] @@ -708,6 +724,7 @@ mod tests { // (40|-----------|40) // p3) (b1+|-----------|n3) l=67 + let g = graph_b(); let new_ids = expected_new_node_ids_b(); let blocks = btreemap! { @@ -719,11 +736,11 @@ mod tests { }; let nodes = btreemap! { - new_ids[&NodeId(1)] => PangraphNode::new(None, BlockId(1), PathId(1), Reverse, (0 , 61)), - new_ids[&NodeId(2)] => PangraphNode::new(None, BlockId(1), PathId(2), Reverse, (10, 72)), - new_ids[&NodeId(3)] => PangraphNode::new(None, BlockId(1), PathId(3), Forward, (5, 5)), - NodeId(7) => PangraphNode::new(Some(NodeId(7)), BlockId(3), PathId(1), Forward, (61, 0 )), - NodeId(8) => PangraphNode::new(Some(NodeId(8)), BlockId(3), PathId(2), Forward, (72, 10)), + new_ids[&NodeId(1)] => derived_node(&g, BlockId(1), PathId(1), Reverse, (0 , 61)), + new_ids[&NodeId(2)] => derived_node(&g, BlockId(1), PathId(2), Reverse, (10, 72)), + new_ids[&NodeId(3)] => derived_node(&g, BlockId(1), PathId(3), Forward, (5, 5)), + NodeId(7) => PangraphNode::new(NodeId(7), BlockId(3), PathId(1), Forward, (61, 0 )), + NodeId(8) => PangraphNode::new(NodeId(8), BlockId(3), PathId(2), Forward, (72, 10)), }; #[rustfmt::skip] @@ -744,6 +761,7 @@ mod tests { // (40|-----------|40) // p3) (b1-|-----------|n3) l=67 + let g = graph_c(); let new_ids = expected_new_node_ids_c(); let blocks = btreemap! { @@ -755,11 +773,11 @@ mod tests { }; let nodes = btreemap! { - new_ids[&NodeId(1)] => PangraphNode::new(None, BlockId(1), PathId(1), Forward, (0 , 61)), - new_ids[&NodeId(2)] => PangraphNode::new(None, BlockId(1), PathId(2), Forward, (10, 72)), - new_ids[&NodeId(3)] => PangraphNode::new(None, BlockId(1), PathId(3), Reverse, (5, 5)), - NodeId(7) => PangraphNode::new(Some(NodeId(7)), BlockId(3), PathId(1), Forward, (61, 0 )), - NodeId(8) => PangraphNode::new(Some(NodeId(8)), BlockId(3), PathId(2), Forward, (72, 10)), + new_ids[&NodeId(1)] => derived_node(&g, BlockId(1), PathId(1), Forward, (0 , 61)), + new_ids[&NodeId(2)] => derived_node(&g, BlockId(1), PathId(2), Forward, (10, 72)), + new_ids[&NodeId(3)] => derived_node(&g, BlockId(1), PathId(3), Reverse, (5, 5)), + NodeId(7) => PangraphNode::new(NodeId(7), BlockId(3), PathId(1), Forward, (61, 0 )), + NodeId(8) => PangraphNode::new(NodeId(8), BlockId(3), PathId(2), Forward, (72, 10)), }; #[rustfmt::skip] diff --git a/packages/pangraph/src/commands/simplify/simplify_run.rs b/packages/pangraph/src/commands/simplify/simplify_run.rs index f38d5a17..04a6215b 100644 --- a/packages/pangraph/src/commands/simplify/simplify_run.rs +++ b/packages/pangraph/src/commands/simplify/simplify_run.rs @@ -133,14 +133,14 @@ mod tests { // n2+ -> n5+ -> n8- // n3+ -> n6- let nodes = btreemap! { - NodeId(1) => PangraphNode::new(Some(NodeId(1)), BlockId(1), PathId(1), Forward, (0, 32)), - NodeId(2) => PangraphNode::new(Some(NodeId(2)), BlockId(1), PathId(2), Forward, (0, 31)), - NodeId(3) => PangraphNode::new(Some(NodeId(3)), BlockId(1), PathId(3), Forward, (0, 35)), - NodeId(4) => PangraphNode::new(Some(NodeId(4)), BlockId(2), PathId(1), Forward, (32, 64)), - NodeId(5) => PangraphNode::new(Some(NodeId(5)), BlockId(2), PathId(2), Forward, (31, 60)), - NodeId(6) => PangraphNode::new(Some(NodeId(6)), BlockId(2), PathId(3), Forward, (35, 0)), - NodeId(7) => PangraphNode::new(Some(NodeId(7)), BlockId(3), PathId(1), Forward, (64, 0)), - NodeId(8) => PangraphNode::new(Some(NodeId(8)), BlockId(3), PathId(2), Reverse, (60, 0)), + NodeId(1) => PangraphNode::new(NodeId(1), BlockId(1), PathId(1), Forward, (0, 32)), + NodeId(2) => PangraphNode::new(NodeId(2), BlockId(1), PathId(2), Forward, (0, 31)), + NodeId(3) => PangraphNode::new(NodeId(3), BlockId(1), PathId(3), Forward, (0, 35)), + NodeId(4) => PangraphNode::new(NodeId(4), BlockId(2), PathId(1), Forward, (32, 64)), + NodeId(5) => PangraphNode::new(NodeId(5), BlockId(2), PathId(2), Forward, (31, 60)), + NodeId(6) => PangraphNode::new(NodeId(6), BlockId(2), PathId(3), Forward, (35, 0)), + NodeId(7) => PangraphNode::new(NodeId(7), BlockId(3), PathId(1), Forward, (64, 0)), + NodeId(8) => PangraphNode::new(NodeId(8), BlockId(3), PathId(2), Reverse, (60, 0)), }; let blocks = btreemap! { BlockId(1) => block_a(), @@ -157,10 +157,10 @@ mod tests { fn expected_graph() -> Pangraph { let nodes = btreemap! { - NID11 => PangraphNode::new(Some(NodeId(11)), BlockId(1), PathId(1), Forward, (0, 64)), - NID12 => PangraphNode::new(Some(NodeId(12)), BlockId(1), PathId(2), Forward, (0, 60)), - NodeId(7) => PangraphNode::new(Some(NodeId(7)), BlockId(3), PathId(1), Forward, (64, 0)), - NodeId(8) => PangraphNode::new(Some(NodeId(8)), BlockId(3), PathId(2), Reverse, (60, 0)), + NID11 => PangraphNode::new(NodeId(11), BlockId(1), PathId(1), Forward, (0, 64)), + NID12 => PangraphNode::new(NodeId(12), BlockId(1), PathId(2), Forward, (0, 60)), + NodeId(7) => PangraphNode::new(NodeId(7), BlockId(3), PathId(1), Forward, (64, 0)), + NodeId(8) => PangraphNode::new(NodeId(8), BlockId(3), PathId(2), Reverse, (60, 0)), }; let blocks = btreemap! { BlockId(1) => block_ab(), @@ -186,11 +186,11 @@ mod tests { }; let expected_nodes = btreemap! { - NodeId(2) => PangraphNode::new(Some(NodeId(2)), BlockId(1), PathId(2), Forward, (0, 31)), - NodeId(3) => PangraphNode::new(Some(NodeId(3)), BlockId(1), PathId(3), Forward, (0, 35)), - NodeId(5) => PangraphNode::new(Some(NodeId(5)), BlockId(2), PathId(2), Forward, (31, 60)), - NodeId(6) => PangraphNode::new(Some(NodeId(6)), BlockId(2), PathId(3), Forward, (35, 0)), - NodeId(8) => PangraphNode::new(Some(NodeId(8)), BlockId(3), PathId(2), Reverse, (60, 0)), + NodeId(2) => PangraphNode::new(NodeId(2), BlockId(1), PathId(2), Forward, (0, 31)), + NodeId(3) => PangraphNode::new(NodeId(3), BlockId(1), PathId(3), Forward, (0, 35)), + NodeId(5) => PangraphNode::new(NodeId(5), BlockId(2), PathId(2), Forward, (31, 60)), + NodeId(6) => PangraphNode::new(NodeId(6), BlockId(2), PathId(3), Forward, (35, 0)), + NodeId(8) => PangraphNode::new(NodeId(8), BlockId(3), PathId(2), Reverse, (60, 0)), }; let expected_blocks = btreemap! { diff --git a/packages/pangraph/src/pangraph/detach_unaligned.rs b/packages/pangraph/src/pangraph/detach_unaligned.rs index ee33c6e6..e718721b 100644 --- a/packages/pangraph/src/pangraph/detach_unaligned.rs +++ b/packages/pangraph/src/pangraph/detach_unaligned.rs @@ -103,7 +103,7 @@ fn create_new_node_and_block( // Create a new PangraphNode for the unaligned node let new_node = PangraphNode::new( - Some(node_id), + node_id, new_block_id, old_node.path_id(), // same path ID as the old node Forward, // assuming the new node is always on the forward strand @@ -132,11 +132,11 @@ mod tests { let seq = Seq::from_str("ATGTTGATAG"); let old_block_id = BlockId(0); let old_path_id = PathId(0); - let old_node = PangraphNode::new(Some(node_id), old_block_id, old_path_id, Forward, (10, 20)); + let old_node = PangraphNode::new(node_id, old_block_id, old_path_id, Forward, (10, 20)); let (new_node, new_block) = create_new_node_and_block(node_id, seq.clone(), &old_node)?; - let expected_new_node = PangraphNode::new(Some(node_id), new_block.id(), old_path_id, Forward, (10, 20)); + let expected_new_node = PangraphNode::new(node_id, new_block.id(), old_path_id, Forward, (10, 20)); let expected_new_block = PangraphBlock::from_consensus(seq, new_block.id(), node_id); assert_eq!(new_node, expected_new_node); @@ -151,11 +151,11 @@ mod tests { let seq = Seq::from_str("ATGTTGATAG"); let old_block_id = BlockId(0); let old_path_id = PathId(1); - let old_node = PangraphNode::new(Some(node_id), old_block_id, old_path_id, Reverse, (5, 15)); + let old_node = PangraphNode::new(node_id, old_block_id, old_path_id, Reverse, (5, 15)); let (new_node, new_block) = create_new_node_and_block(node_id, seq.clone(), &old_node)?; - let expected_new_node = PangraphNode::new(Some(node_id), new_block.id(), old_path_id, Forward, (5, 15)); + let expected_new_node = PangraphNode::new(node_id, new_block.id(), old_path_id, Forward, (5, 15)); let expected_new_block = PangraphBlock::from_consensus(reverse_complement(&seq)?, new_block.id(), node_id); assert_eq!(new_node, expected_new_node); @@ -211,8 +211,8 @@ mod tests { }, ); let mut blocks = vec![block]; - let node1 = PangraphNode::new(Some(NodeId(1)), BlockId(0), PathId(0), Forward, (0, 16)); - let node2 = PangraphNode::new(Some(NodeId(2)), BlockId(0), PathId(1), Reverse, (0, 8)); + let node1 = PangraphNode::new(NodeId(1), BlockId(0), PathId(0), Forward, (0, 16)); + let node2 = PangraphNode::new(NodeId(2), BlockId(0), PathId(1), Reverse, (0, 8)); let mut nodes = btreemap! { NodeId(1) => node1, NodeId(2) => node2, @@ -231,8 +231,8 @@ mod tests { assert_eq!(blocks[1], expected_block2); let expected_node_dict = btreemap! { - NodeId(1) => PangraphNode::new(Some(NodeId(1)), BlockId(0), PathId(0), Forward, (0, 16)), - NodeId(2) => PangraphNode::new(Some(NodeId(2)), new_block_id, PathId(1), Forward, (0, 8)), + NodeId(1) => PangraphNode::new(NodeId(1), BlockId(0), PathId(0), Forward, (0, 16)), + NodeId(2) => PangraphNode::new(NodeId(2), new_block_id, PathId(1), Forward, (0, 8)), }; assert_eq!(nodes, expected_node_dict); Ok(()) diff --git a/packages/pangraph/src/pangraph/pangraph.rs b/packages/pangraph/src/pangraph/pangraph.rs index 1a7c6b1f..9b90e1d2 100644 --- a/packages/pangraph/src/pangraph/pangraph.rs +++ b/packages/pangraph/src/pangraph/pangraph.rs @@ -42,7 +42,7 @@ impl Pangraph { let block = PangraphBlock::from_consensus(fasta.seq, block_id, node_id); let path_id = PathId(fasta.index); let node_position = if circular { (0, 0) } else { (0, tot_len) }; // path wraps around if circular - let node = PangraphNode::new(Some(node_id), block.id(), path_id, strand, node_position); + let node = PangraphNode::new(node_id, block.id(), path_id, strand, node_position); let path = PangraphPath::new( Some(path_id), [node.id()], @@ -121,7 +121,7 @@ impl Pangraph { ) })?; - let node = PangraphNode::new(Some(nid), node.block_id(), path_id, node.strand(), node.position()); + let node = PangraphNode::new(nid, node.block_id(), path_id, node.strand(), node.position()); Ok((nid, node)) }) .collect::>()?; @@ -493,14 +493,14 @@ mod tests { // b2+ -> [b4+, b5-] let nodes = btreemap! { - NodeId(1) => PangraphNode::new(Some(NodeId(1)), BlockId(1), PathId(1), Forward, (0, 0)), // FIXME - NodeId(2) => PangraphNode::new(Some(NodeId(2)), BlockId(1), PathId(3), Forward, (0, 0)), // FIXME - NodeId(3) => PangraphNode::new(Some(NodeId(3)), BlockId(2), PathId(1), Forward, (0, 0)), // FIXME - NodeId(4) => PangraphNode::new(Some(NodeId(4)), BlockId(2), PathId(2), Forward, (0, 0)), // FIXME - NodeId(5) => PangraphNode::new(Some(NodeId(5)), BlockId(2), PathId(3), Reverse, (0, 0)), // FIXME - NodeId(6) => PangraphNode::new(Some(NodeId(6)), BlockId(3), PathId(1), Forward, (0, 0)), // FIXME - NodeId(7) => PangraphNode::new(Some(NodeId(7)), BlockId(3), PathId(2), Forward, (0, 0)), // FIXME - NodeId(8) => PangraphNode::new(Some(NodeId(8)), BlockId(3), PathId(3), Forward, (0, 0)) // FIXME + NodeId(1) => PangraphNode::new(NodeId(1), BlockId(1), PathId(1), Forward, (0, 0)), // FIXME + NodeId(2) => PangraphNode::new(NodeId(2), BlockId(1), PathId(3), Forward, (0, 0)), // FIXME + NodeId(3) => PangraphNode::new(NodeId(3), BlockId(2), PathId(1), Forward, (0, 0)), // FIXME + NodeId(4) => PangraphNode::new(NodeId(4), BlockId(2), PathId(2), Forward, (0, 0)), // FIXME + NodeId(5) => PangraphNode::new(NodeId(5), BlockId(2), PathId(3), Reverse, (0, 0)), // FIXME + NodeId(6) => PangraphNode::new(NodeId(6), BlockId(3), PathId(1), Forward, (0, 0)), // FIXME + NodeId(7) => PangraphNode::new(NodeId(7), BlockId(3), PathId(2), Forward, (0, 0)), // FIXME + NodeId(8) => PangraphNode::new(NodeId(8), BlockId(3), PathId(3), Forward, (0, 0)) // FIXME }; let blocks = btreemap! { @@ -525,12 +525,12 @@ mod tests { }; let new_nodes = btreemap! { - NodeId(9) => PangraphNode::new(Some(NodeId(9)), BlockId(4), PathId(1), Forward, (0, 0)), - NodeId(10) => PangraphNode::new(Some(NodeId(10)), BlockId(5), PathId(1), Reverse, (0, 0)), - NodeId(11) => PangraphNode::new(Some(NodeId(11)), BlockId(4), PathId(2), Forward, (0, 0)), - NodeId(12) => PangraphNode::new(Some(NodeId(12)), BlockId(5), PathId(2), Reverse, (0, 0)), - NodeId(13) => PangraphNode::new(Some(NodeId(13)), BlockId(4), PathId(3), Reverse, (0, 0)), - NodeId(14) => PangraphNode::new(Some(NodeId(14)), BlockId(5), PathId(3), Forward, (0, 0)), + NodeId(9) => PangraphNode::new(NodeId(9), BlockId(4), PathId(1), Forward, (0, 0)), + NodeId(10) => PangraphNode::new(NodeId(10), BlockId(5), PathId(1), Reverse, (0, 0)), + NodeId(11) => PangraphNode::new(NodeId(11), BlockId(4), PathId(2), Forward, (0, 0)), + NodeId(12) => PangraphNode::new(NodeId(12), BlockId(5), PathId(2), Reverse, (0, 0)), + NodeId(13) => PangraphNode::new(NodeId(13), BlockId(4), PathId(3), Reverse, (0, 0)), + NodeId(14) => PangraphNode::new(NodeId(14), BlockId(5), PathId(3), Forward, (0, 0)), }; let new_blocks = btreemap! { @@ -634,8 +634,8 @@ mod tests { b1 => PangraphBlock::new(b1, "TTTTGGGG", btreemap!{ n1 => Edit::empty() }), }; let nodes = btreemap! { - n0 => PangraphNode::new(Some(n0), b0, PathId(0), Forward, (0, 8)), - n1 => PangraphNode::new(Some(n1), b1, PathId(1), Reverse, (0, 8)), + n0 => PangraphNode::new(n0, b0, PathId(0), Forward, (0, 8)), + n1 => PangraphNode::new(n1, b1, PathId(1), Reverse, (0, 8)), }; let paths = btreemap! { PathId(0) => PangraphPath::new(Some(PathId(0)), [n0], 8, false, Some(names[0].to_owned()), None), @@ -653,7 +653,7 @@ mod tests { let mut graph = two_genome_graph(["a", "b"]); let nid = graph.node_ids().next().unwrap(); let node = &graph.nodes[&nid]; - let detached = PangraphNode::new(Some(nid), node.block_id(), PathId(99), node.strand(), node.position()); + let detached = PangraphNode::new(nid, node.block_id(), PathId(99), node.strand(), node.position()); graph.nodes.insert(nid, detached); let err = report_to_string(&graph.renumber_paths(7).unwrap_err()); @@ -849,7 +849,7 @@ mod tests { let node = &graph.nodes[&nid]; graph.nodes.insert( nid, - PangraphNode::new(Some(nid), BlockId(99), node.path_id(), node.strand(), node.position()), + PangraphNode::new(nid, BlockId(99), node.path_id(), node.strand(), node.position()), ); let err = report_to_string(&graph.validate().unwrap_err()); @@ -866,7 +866,7 @@ mod tests { let node = &graph.nodes[&nid]; graph.nodes.insert( nid, - PangraphNode::new(Some(nid), node.block_id(), PathId(99), node.strand(), node.position()), + PangraphNode::new(nid, node.block_id(), PathId(99), node.strand(), node.position()), ); let err = report_to_string(&graph.validate().unwrap_err()); @@ -931,7 +931,7 @@ mod tests { let node = &graph.nodes[&nid]; graph.nodes.insert( nid, - PangraphNode::new(Some(nid), node.block_id(), node.path_id(), node.strand(), (100, 8)), + PangraphNode::new(nid, node.block_id(), node.path_id(), node.strand(), (100, 8)), ); let err = report_to_string(&graph.validate().unwrap_err()); diff --git a/packages/pangraph/src/pangraph/pangraph_node.rs b/packages/pangraph/src/pangraph/pangraph_node.rs index 91498ce9..55419867 100644 --- a/packages/pangraph/src/pangraph/pangraph_node.rs +++ b/packages/pangraph/src/pangraph/pangraph_node.rs @@ -35,14 +35,13 @@ impl NodeId { } impl PangraphNode { - pub fn new( - node_id: Option, - block_id: BlockId, - path_id: PathId, - strand: Strand, - position: (usize, usize), - ) -> Self { - let id = node_id.unwrap_or_else(|| id((&block_id, &path_id, &strand, &position))); + /// Creates a node with an explicit id. + /// + /// Node ids are content-derived, and [`PangraphNode::with_derived_id`] is the one place that + /// derives them. This constructor takes the id it is given, so a caller that has an id already — + /// because the node is being rewritten rather than created — cannot accidentally seed a second, + /// divergent derivation scheme. + pub fn new(id: NodeId, block_id: BlockId, path_id: PathId, strand: Strand, position: (usize, usize)) -> Self { Self { id, block_id, @@ -54,9 +53,9 @@ impl PangraphNode { /// Creates a node placing `block_id` on `path`, with an id derived from its contents. /// - /// Genomes are discriminated by [`PangraphPath::seed`] rather than by their path id, so the - /// resulting id does not depend on the order in which the input sequences were read. The path id - /// is still what the node stores. + /// This is the only place a node id is derived. Genomes are discriminated by + /// [`PangraphPath::seed`] rather than by their path id, so the resulting id does not depend on + /// the order in which the input sequences were read. The path id is still what the node stores. pub fn with_derived_id(block_id: BlockId, path: &PangraphPath, strand: Strand, position: (usize, usize)) -> Self { Self { id: id((&block_id, &path.seed(), &strand, &position)), diff --git a/packages/pangraph/src/pangraph/reconstruct.rs b/packages/pangraph/src/pangraph/reconstruct.rs index 9a7920bb..3c0b8921 100644 --- a/packages/pangraph/src/pangraph/reconstruct.rs +++ b/packages/pangraph/src/pangraph/reconstruct.rs @@ -394,8 +394,8 @@ mod tests { BlockId(1) => PangraphBlock::new(BlockId(1), "TTTTGGGG", btreemap!{ NodeId(1) => Edit::empty() }), }; let nodes = btreemap! { - NodeId(0) => PangraphNode::new(Some(NodeId(0)), BlockId(0), PathId(0), Forward, (0, 8)), - NodeId(1) => PangraphNode::new(Some(NodeId(1)), BlockId(1), PathId(1), Reverse, (0, 8)), + NodeId(0) => PangraphNode::new(NodeId(0), BlockId(0), PathId(0), Forward, (0, 8)), + NodeId(1) => PangraphNode::new(NodeId(1), BlockId(1), PathId(1), Reverse, (0, 8)), }; let paths = btreemap! { PathId(0) => PangraphPath::new(Some(PathId(0)), [NodeId(0)], 8, false, names[0].map(String::from), None), @@ -413,7 +413,7 @@ mod tests { BlockId(0) => PangraphBlock::new(BlockId(0), consensus, btreemap!{ NodeId(0) => Edit::empty() }), }; let nodes = btreemap! { - NodeId(0) => PangraphNode::new(Some(NodeId(0)), BlockId(0), PathId(0), strand, (0, len)), + NodeId(0) => PangraphNode::new(NodeId(0), BlockId(0), PathId(0), strand, (0, len)), }; let paths = btreemap! { PathId(0) => PangraphPath::new(Some(PathId(0)), [NodeId(0)], len, false, Some(name.to_owned()), None), @@ -678,13 +678,7 @@ mod tests { let node = &graph.nodes[&NodeId(0)]; graph.nodes.insert( NodeId(0), - PangraphNode::new( - Some(NodeId(0)), - node.block_id(), - node.path_id(), - node.strand(), - (100, 8), - ), + PangraphNode::new(NodeId(0), node.block_id(), node.path_id(), node.strand(), (100, 8)), ); let err = report_to_string(&reconstruct_genome(&graph, PathId(0)).unwrap_err()); diff --git a/packages/pangraph/src/pangraph/reweave.rs b/packages/pangraph/src/pangraph/reweave.rs index 8bc07213..74856317 100644 --- a/packages/pangraph/src/pangraph/reweave.rs +++ b/packages/pangraph/src/pangraph/reweave.rs @@ -704,9 +704,9 @@ mod tests { let nid2 = NodeId(2000); let nid3 = NodeId(3000); - let n1 = PangraphNode::new(Some(nid1), bid, PathId(100), Forward, (100, 230)); - let n2 = PangraphNode::new(Some(nid2), bid, PathId(200), Reverse, (1000, 1130)); - let n3 = PangraphNode::new(Some(nid3), bid, PathId(300), Reverse, (180, 110)); + let n1 = PangraphNode::new(nid1, bid, PathId(100), Forward, (100, 230)); + let n2 = PangraphNode::new(nid2, bid, PathId(200), Reverse, (1000, 1130)); + let n3 = PangraphNode::new(nid3, bid, PathId(300), Reverse, (180, 110)); let b1 = PangraphBlock::new( bid, @@ -885,14 +885,14 @@ mod tests { fn generate_example() -> (Pangraph, Vec) { let nodes = btreemap! { - NodeId(1) => PangraphNode::new(Some(NodeId(1)), BlockId(10), PathId(100), Forward, (700, 885)), - NodeId(2) => PangraphNode::new(Some(NodeId(2)), BlockId(30), PathId(100), Forward, (885, 988)), - NodeId(3) => PangraphNode::new(Some(NodeId(3)), BlockId(30), PathId(200), Reverse, (100, 180)), - NodeId(4) => PangraphNode::new(Some(NodeId(4)), BlockId(20), PathId(200), Reverse, (180, 555)), - NodeId(5) => PangraphNode::new(Some(NodeId(5)), BlockId(10), PathId(200), Reverse, (555, 735)), - NodeId(6) => PangraphNode::new(Some(NodeId(6)), BlockId(40), PathId(300), Forward, (600, 100)), - NodeId(7) => PangraphNode::new(Some(NodeId(7)), BlockId(50), PathId(300), Forward, (100, 325)), - NodeId(8) => PangraphNode::new(Some(NodeId(8)), BlockId(50), PathId(300), Reverse, (325, 580)), + NodeId(1) => PangraphNode::new(NodeId(1), BlockId(10), PathId(100), Forward, (700, 885)), + NodeId(2) => PangraphNode::new(NodeId(2), BlockId(30), PathId(100), Forward, (885, 988)), + NodeId(3) => PangraphNode::new(NodeId(3), BlockId(30), PathId(200), Reverse, (100, 180)), + NodeId(4) => PangraphNode::new(NodeId(4), BlockId(20), PathId(200), Reverse, (180, 555)), + NodeId(5) => PangraphNode::new(NodeId(5), BlockId(10), PathId(200), Reverse, (555, 735)), + NodeId(6) => PangraphNode::new(NodeId(6), BlockId(40), PathId(300), Forward, (600, 100)), + NodeId(7) => PangraphNode::new(NodeId(7), BlockId(50), PathId(300), Forward, (100, 325)), + NodeId(8) => PangraphNode::new(NodeId(8), BlockId(50), PathId(300), Reverse, (325, 580)), }; let paths = btreemap! { diff --git a/packages/pangraph/src/pangraph/slice.rs b/packages/pangraph/src/pangraph/slice.rs index 98e8fd50..25bb23a8 100644 --- a/packages/pangraph/src/pangraph/slice.rs +++ b/packages/pangraph/src/pangraph/slice.rs @@ -468,15 +468,15 @@ mod tests { let (new_b, new_nodes) = block_slice(&b, &i, &G); assert_eq!(new_b.consensus(), "TATATTTATC"); - let nn1 = PangraphNode::new(None, new_bid, PathId(1), Forward, (111, 120)); + let nn1 = PangraphNode::with_derived_id(new_bid, &G.paths[&PathId(1)], Forward, (111, 120)); let nn1_slice = new_nodes[&NodeId(1)].as_ref().unwrap(); assert_eq!(nn1, *nn1_slice); - let nn2 = PangraphNode::new(None, new_bid, PathId(2), Reverse, (1008, 1017)); + let nn2 = PangraphNode::with_derived_id(new_bid, &G.paths[&PathId(2)], Reverse, (1008, 1017)); let nn2_slice = new_nodes[&NodeId(2)].as_ref().unwrap(); assert_eq!(nn2, *nn2_slice); - let nn3 = PangraphNode::new(None, new_bid, PathId(3), Reverse, (96, 4)); + let nn3 = PangraphNode::with_derived_id(new_bid, &G.paths[&PathId(3)], Reverse, (96, 4)); let nn3_slice = new_nodes[&NodeId(3)].as_ref().unwrap(); assert_eq!(nn3, *nn3_slice); @@ -533,9 +533,9 @@ mod tests { inss: vec![Ins::new(20, "T")], }; - let n1 = PangraphNode::new(Some(NodeId(1)), bid, PathId(1), Forward, (100, 125)); - let n2 = PangraphNode::new(Some(NodeId(2)), bid, PathId(2), Reverse, (1000, 1025)); - let n3 = PangraphNode::new(Some(NodeId(3)), bid, PathId(3), Reverse, (90, 9)); + let n1 = PangraphNode::new(NodeId(1), bid, PathId(1), Forward, (100, 125)); + let n2 = PangraphNode::new(NodeId(2), bid, PathId(2), Reverse, (1000, 1025)); + let n3 = PangraphNode::new(NodeId(3), bid, PathId(3), Reverse, (90, 9)); let p1 = PangraphPath::new( Some(PathId(1)), @@ -610,15 +610,15 @@ mod tests { assert_eq!(new_b.consensus(), "TATATTTATC"); - let nn1 = PangraphNode::new(None, new_bid, PathId(1), Reverse, (111, 120)); + let nn1 = PangraphNode::with_derived_id(new_bid, &G.paths[&PathId(1)], Reverse, (111, 120)); let nn1_slice = new_nodes[&NodeId(1)].as_ref().unwrap(); assert_eq!(nn1, *nn1_slice); - let nn2 = PangraphNode::new(None, new_bid, PathId(2), Forward, (1008, 1017)); + let nn2 = PangraphNode::with_derived_id(new_bid, &G.paths[&PathId(2)], Forward, (1008, 1017)); let nn2_slice = new_nodes[&NodeId(2)].as_ref().unwrap(); assert_eq!(nn2, *nn2_slice); - let nn3 = PangraphNode::new(None, new_bid, PathId(3), Forward, (96, 4)); + let nn3 = PangraphNode::with_derived_id(new_bid, &G.paths[&PathId(3)], Forward, (96, 4)); let nn3_slice = new_nodes[&NodeId(3)].as_ref().unwrap(); assert_eq!(nn3, *nn3_slice); diff --git a/packages/pangraph/src/reconsensus/reconsensus.rs b/packages/pangraph/src/reconsensus/reconsensus.rs index 1a53bee2..1666770c 100644 --- a/packages/pangraph/src/reconsensus/reconsensus.rs +++ b/packages/pangraph/src/reconsensus/reconsensus.rs @@ -436,11 +436,11 @@ mod tests { let expected_block = block_1_reconsensus(); let nodes = btreemap! { - NodeId(1) => PangraphNode::new(Some(NodeId(1)), block.id(), PathId(1), Forward, (0, 23)), - NodeId(2) => PangraphNode::new(Some(NodeId(2)), block.id(), PathId(2), Forward, (0, 23)), - NodeId(3) => PangraphNode::new(Some(NodeId(3)), block.id(), PathId(3), Forward, (0, 23)), - NodeId(4) => PangraphNode::new(Some(NodeId(4)), block.id(), PathId(4), Forward, (0, 23)), - NodeId(5) => PangraphNode::new(Some(NodeId(5)), block.id(), PathId(5), Forward, (0, 23)), + NodeId(1) => PangraphNode::new(NodeId(1), block.id(), PathId(1), Forward, (0, 23)), + NodeId(2) => PangraphNode::new(NodeId(2), block.id(), PathId(2), Forward, (0, 23)), + NodeId(3) => PangraphNode::new(NodeId(3), block.id(), PathId(3), Forward, (0, 23)), + NodeId(4) => PangraphNode::new(NodeId(4), block.id(), PathId(4), Forward, (0, 23)), + NodeId(5) => PangraphNode::new(NodeId(5), block.id(), PathId(5), Forward, (0, 23)), }; let paths = btreemap! { PathId(1) => PangraphPath::new(Some(PathId(1)), [NodeId(1)], 23, false, None, None), @@ -510,11 +510,11 @@ mod tests { // Create nodes for the block with lengths reflecting actual sequence lengths let nodes = btreemap! { - NodeId(1) => PangraphNode::new(Some(NodeId(1)), initial_block.id(), PathId(1), Reverse, (0, 10)), // 50 - 40 = 9 (deletes positions 0-39) - NodeId(2) => PangraphNode::new(Some(NodeId(2)), initial_block.id(), PathId(2), Forward, (0, 35)), // 50 - 15 = 35 (deletes positions 35-49) - NodeId(3) => PangraphNode::new(Some(NodeId(3)), initial_block.id(), PathId(3), Forward, (0, 35)), // 50 - 15 = 35 (deletes positions 35-49) - NodeId(4) => PangraphNode::new(Some(NodeId(4)), initial_block.id(), PathId(4), Forward, (0, 35)), // 50 - 15 = 35 (deletes positions 35-49) - NodeId(5) => PangraphNode::new(Some(NodeId(5)), initial_block.id(), PathId(5), Forward, (0, 49)), // no deletions + NodeId(1) => PangraphNode::new(NodeId(1), initial_block.id(), PathId(1), Reverse, (0, 10)), // 50 - 40 = 9 (deletes positions 0-39) + NodeId(2) => PangraphNode::new(NodeId(2), initial_block.id(), PathId(2), Forward, (0, 35)), // 50 - 15 = 35 (deletes positions 35-49) + NodeId(3) => PangraphNode::new(NodeId(3), initial_block.id(), PathId(3), Forward, (0, 35)), // 50 - 15 = 35 (deletes positions 35-49) + NodeId(4) => PangraphNode::new(NodeId(4), initial_block.id(), PathId(4), Forward, (0, 35)), // 50 - 15 = 35 (deletes positions 35-49) + NodeId(5) => PangraphNode::new(NodeId(5), initial_block.id(), PathId(5), Forward, (0, 49)), // no deletions }; // Create paths @@ -554,7 +554,7 @@ mod tests { // check that the node was updated correctly, flipping the strandedness let new_node1 = &graph.nodes[&NodeId(1)]; - let expected_node1 = PangraphNode::new(Some(NodeId(1)), singleton_block_exp.id(), PathId(1), Forward, (0, 10)); + let expected_node1 = PangraphNode::new(NodeId(1), singleton_block_exp.id(), PathId(1), Forward, (0, 10)); assert_eq!(new_node1, &expected_node1); } } diff --git a/packages/pangraph/src/reconsensus/remove_nodes.rs b/packages/pangraph/src/reconsensus/remove_nodes.rs index cf9b88b3..5ed3baaa 100644 --- a/packages/pangraph/src/reconsensus/remove_nodes.rs +++ b/packages/pangraph/src/reconsensus/remove_nodes.rs @@ -87,11 +87,11 @@ mod tests { fn create_input_graph() -> Pangraph { let nodes = btreemap! { - NodeId(1) => PangraphNode::new(Some(NodeId(1)), BlockId(1), PathId(0), Forward, (0, 10)), - NodeId(2) => PangraphNode::new(Some(NodeId(2)), BlockId(1), PathId(1), Forward, (0, 10)), - NodeId(3) => PangraphNode::new(Some(NodeId(3)), BlockId(1), PathId(2), Reverse, (0, 0)), - NodeId(4) => PangraphNode::new(Some(NodeId(4)), BlockId(2), PathId(0), Forward, (10, 20)), - NodeId(5) => PangraphNode::new(Some(NodeId(5)), BlockId(2), PathId(2), Forward, (0, 10)), + NodeId(1) => PangraphNode::new(NodeId(1), BlockId(1), PathId(0), Forward, (0, 10)), + NodeId(2) => PangraphNode::new(NodeId(2), BlockId(1), PathId(1), Forward, (0, 10)), + NodeId(3) => PangraphNode::new(NodeId(3), BlockId(1), PathId(2), Reverse, (0, 0)), + NodeId(4) => PangraphNode::new(NodeId(4), BlockId(2), PathId(0), Forward, (10, 20)), + NodeId(5) => PangraphNode::new(NodeId(5), BlockId(2), PathId(2), Forward, (0, 10)), }; let paths = btreemap! { diff --git a/packages/pangraph/tests/itest_merge.rs b/packages/pangraph/tests/itest_merge.rs index 7b3bc9cf..13d96d39 100644 --- a/packages/pangraph/tests/itest_merge.rs +++ b/packages/pangraph/tests/itest_merge.rs @@ -251,7 +251,7 @@ mod tests { let (bid, nid, pid) = (BlockId(0), NodeId(0), PathId(0)); Pangraph { blocks: BTreeMap::from([(bid, PangraphBlock::from_consensus(seq, bid, nid))]), - nodes: BTreeMap::from([(nid, PangraphNode::new(Some(nid), bid, pid, Forward, (0, seq.len())))]), + nodes: BTreeMap::from([(nid, PangraphNode::new(nid, bid, pid, Forward, (0, seq.len())))]), paths: BTreeMap::from([( pid, PangraphPath::new(Some(pid), [nid], seq.len(), false, Some(name.to_owned()), None), diff --git a/packages/pangraph/tests/itest_reconstruct.rs b/packages/pangraph/tests/itest_reconstruct.rs index 96e703b1..10134fb3 100644 --- a/packages/pangraph/tests/itest_reconstruct.rs +++ b/packages/pangraph/tests/itest_reconstruct.rs @@ -182,7 +182,7 @@ mod tests { BlockId(0) => PangraphBlock::new(BlockId(0), "ACGTACGT", btreemap!{ NodeId(0) => Edit::empty() }), }, nodes: btreemap! { - NodeId(0) => PangraphNode::new(Some(NodeId(0)), BlockId(0), PathId(0), Forward, (0, 8)), + NodeId(0) => PangraphNode::new(NodeId(0), BlockId(0), PathId(0), Forward, (0, 8)), }, paths: btreemap! { PathId(0) => PangraphPath::new(Some(PathId(0)), [NodeId(0)], 8, false, None, None), @@ -250,7 +250,7 @@ mod tests { BlockId(0) => PangraphBlock::new(BlockId(0), "ACGTACGT", btreemap! { NodeId(0) => Edit::empty() }), }, nodes: btreemap! { - NodeId(0) => PangraphNode::new(Some(NodeId(0)), BlockId(0), PathId(0), Forward, (0, 8)), + NodeId(0) => PangraphNode::new(NodeId(0), BlockId(0), PathId(0), Forward, (0, 8)), }, paths: btreemap! { PathId(0) => PangraphPath::new(Some(PathId(0)), [NodeId(0)], 8, false, Some("a".to_owned()), None), From 3c64030924fbbb9febd49b12c661302581a51e3e Mon Sep 17 00:00:00 2001 From: Marco Molari Date: Tue, 18 Aug 2026 11:29:39 +0200 Subject: [PATCH 4/6] docs: improve error message for identifier collisions in input graphs --- packages/pangraph/src/commands/merge/merge_run.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/pangraph/src/commands/merge/merge_run.rs b/packages/pangraph/src/commands/merge/merge_run.rs index a9bb317d..edb8dd86 100644 --- a/packages/pangraph/src/commands/merge/merge_run.rs +++ b/packages/pangraph/src/commands/merge/merge_run.rs @@ -29,7 +29,7 @@ pub fn merge_run(args: &PangraphMergeArgs) -> Result<(), Report> { // Cheap, and the alternative is `graph_join` panicking on the conflicting key. if !right.is_id_disjoint_from(&left) { return make_error!( - "The two input graphs share block or node identifiers, so they cannot be joined. Identifiers are derived from genome names since version 1.4; graphs written by earlier versions derive them from the order of the input sequences instead, and two such graphs collide. Rebuild the input graphs with the current version of pangraph." + "The two input graphs share block or node identifiers, so they cannot be joined. Identifier collision avoidance is implemented since v1.4.0. If you built your graphs with a previous version of pangraph try rebuilding them. If the error persists please submit an issue." ); } From 7c6a77001cf731311708a95180567f1e1bd8ec70 Mon Sep 17 00:00:00 2001 From: Marco Molari Date: Tue, 18 Aug 2026 11:43:01 +0200 Subject: [PATCH 5/6] fix(pangraph): reject genome names with leading or trailing whitespace --- packages/pangraph/src/pangraph/reconstruct.rs | 86 ++++++++++++++++++- 1 file changed, 84 insertions(+), 2 deletions(-) diff --git a/packages/pangraph/src/pangraph/reconstruct.rs b/packages/pangraph/src/pangraph/reconstruct.rs index 3c0b8921..236f81d0 100644 --- a/packages/pangraph/src/pangraph/reconstruct.rs +++ b/packages/pangraph/src/pangraph/reconstruct.rs @@ -63,7 +63,10 @@ pub fn reconstruct(graph: &Pangraph) -> impl Iterator Result<(), Report> { ); } + let padded = graphs + .iter() + .flat_map(|graph| graph.path_names().flatten()) + .filter(|name| name.trim() != *name) + .map(|name| format!("{name:?}")) + .collect_vec(); + + if !padded.is_empty() { + return make_error!( + "Found {} genome name(s) with leading or trailing whitespace: {}. Genome names are compared exactly, so surrounding whitespace makes a genome impossible to tell apart from, and impossible to address as, the name it appears to have.", + padded.len(), + format_names(&padded) + ); + } + let duplicates = find_duplicates(graphs.iter().flat_map(|graph| graph.path_names().flatten())); if !duplicates.is_empty() { return make_error!( @@ -130,7 +148,8 @@ pub fn reconstruct_genome(graph: &Pangraph, path_id: PathId) -> Result Result<(), Report> { let empty = fastas .iter() @@ -146,6 +165,20 @@ pub fn check_sequence_names(fastas: &[FastaRecord]) -> Result<(), Report> { ); } + let padded = fastas + .iter() + .filter(|fasta| fasta.seq_name.trim() != fasta.seq_name) + .map(|fasta| format!("{:?}", fasta.seq_name)) + .collect_vec(); + + if !padded.is_empty() { + return make_error!( + "Found {} input sequence(s) whose name has leading or trailing whitespace: {}. Sequence names are compared exactly, so surrounding whitespace makes a genome impossible to tell apart from the name it appears to have. Note that whitespace between '>' and the identifier becomes part of the name, unless it is a plain space, which makes the identifier part of the description instead.", + padded.len(), + format_names(&padded) + ); + } + let duplicates = find_duplicates(fastas.iter().map(|fasta| fasta.seq_name.as_str())); if !duplicates.is_empty() { return make_error!( @@ -454,6 +487,34 @@ mod tests { assert!(err.contains("no name or an empty name"), "unexpected error: {err}"); } + /// Every comparison on genome names is exact, so " a" would be a genome that `--strains a` + /// cannot address, that does not collide with the "a" of the graph it is merged with, and that + /// round-trips to a FASTA header with an invisible space in it. + #[rstest] + #[case(" a")] + #[case("a ")] + #[case("\ta")] + fn test_path_ids_by_name_rejects_padded_path_name(#[case] name: &str) { + let graph = two_genome_graph([Some("b"), Some(name)]); + let err = report_to_string(&path_ids_by_name(&graph).unwrap_err()); + assert!( + err.contains("leading or trailing whitespace"), + "unexpected error: {err}" + ); + } + + /// The pair a padded name is most likely to be mistaken for: without the check they are two + /// distinct genomes, since duplicates are detected on the name as given. + #[rstest] + fn test_path_ids_by_name_rejects_a_name_that_differs_only_by_padding() { + let graph = two_genome_graph([Some("a"), Some("a ")]); + let err = report_to_string(&path_ids_by_name(&graph).unwrap_err()); + assert!( + err.contains("leading or trailing whitespace"), + "unexpected error: {err}" + ); + } + #[rstest] fn test_path_ids_by_name_rejects_duplicate_names() { let graph = two_genome_graph([Some("a"), Some("a")]); @@ -505,6 +566,27 @@ mod tests { assert!(err.contains('3'), "unexpected error: {err}"); } + /// A header indented with anything other than a plain space — `>\tid` — keeps the whitespace in + /// the name, which is invisible in every place the name is later shown. + #[rstest] + #[case(" a")] + #[case("a ")] + #[case("\ta")] + fn test_check_sequence_names_rejects_padded_name(#[case] name: &str) { + let fastas = [FastaRecord { + seq_name: name.to_owned(), + desc: None, + seq: Seq::from_str("ACGT"), + index: 3, + }]; + + let err = report_to_string(&check_sequence_names(&fastas).unwrap_err()); + assert!( + err.contains("leading or trailing whitespace"), + "unexpected error: {err}" + ); + } + #[rstest] fn test_verify_graph_sequences_accepts_exact_match() { verify_graph_sequences(&graph(), &expected_genomes(), GenomeCoverage::Complete).unwrap(); From fa876df8c6b3af49c7183b7bcc6d5810ecad5d7c Mon Sep 17 00:00:00 2001 From: Marco Molari Date: Tue, 18 Aug 2026 11:58:42 +0200 Subject: [PATCH 6/6] style: use plain ascii punctuation in comments and docs --- CHANGELOG.md | 2 +- docs/docs/tutorial/t04b-merging-two-graphs.md | 2 +- packages/pangraph/src/commands/merge/merge_run.rs | 8 ++++---- packages/pangraph/src/commands/root_args.rs | 2 +- packages/pangraph/src/pangraph/pangraph.rs | 2 +- packages/pangraph/src/pangraph/pangraph_node.rs | 4 ++-- packages/pangraph/src/pangraph/reconstruct.rs | 10 +++++----- packages/pangraph/tests/itest_reconstruct.rs | 2 +- 8 files changed, 16 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1238f94a..9d595cd5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,7 @@ - added `pangraph merge` command, to combine two existing pangenome graphs into a single one, see #197. - `pangraph build` now accepts a single input sequence, and rejects inputs with duplicate genome names. -- more strict checks on input sequences ids: duplicated or empty ids are now rejected. The `--verify` options compare the input sequence by id and not by order. +- more strict checks on input sequence ids: duplicated or empty ids are now rejected. The `--verify` options compare the input sequences by id and not by order. - added more graph validation at loading. - block and node identifiers are now derived from genome names instead of the order the input sequences are read in, enforcing order-independence. - fixed a race condition in `pangraph build` that could produce slightly different graphs on repeated runs over the same input. diff --git a/docs/docs/tutorial/t04b-merging-two-graphs.md b/docs/docs/tutorial/t04b-merging-two-graphs.md index 76138759..091e7df3 100644 --- a/docs/docs/tutorial/t04b-merging-two-graphs.md +++ b/docs/docs/tutorial/t04b-merging-two-graphs.md @@ -30,7 +30,7 @@ The two graphs can then be merged: pangraph merge graph.json k12.json -o graph_11.json ``` -The resulting `graph_11.json` contains 11 paths: the 10 genomes of `graph.json`, in their original order, followed by K-12. Adding the eleventh chromosome created comparatively few new blocks (2896 → 2943): most of it was absorbed into blocks that already existed. +The resulting `graph_11.json` contains 11 paths: the 10 genomes of `graph.json`, in their original order, followed by K-12. Adding the eleventh chromosome created comparatively few new blocks (2896 -> 2943): most of it was absorbed into blocks that already existed. On a consumer laptop the merge takes around 20 seconds, against the roughly 3.5 minutes needed to build the 10-genome graph in the first place. diff --git a/packages/pangraph/src/commands/merge/merge_run.rs b/packages/pangraph/src/commands/merge/merge_run.rs index edb8dd86..fc89502c 100644 --- a/packages/pangraph/src/commands/merge/merge_run.rs +++ b/packages/pangraph/src/commands/merge/merge_run.rs @@ -71,9 +71,9 @@ pub fn merge_run(args: &PangraphMergeArgs) -> Result<(), Report> { /// Reads a pangraph from a JSON file. /// /// `from_path` already rejects a graph whose ids do not resolve or whose offsets are out of range, -/// in release builds too. The extra `sanity_check` here adds the semantic invariants on top — that -/// node positions tile the genome — which indicate a bug rather than a bad file, and so are only -/// worth paying for in debug builds. +/// in release builds too. The extra `sanity_check` here adds the semantic invariants on top, such +/// as node positions tiling the genome. Those indicate a bug rather than a bad file, and so are +/// only worth paying for in debug builds. fn read_graph(filepath: &Path) -> Result { let graph = Pangraph::from_path(&Some(filepath))?; @@ -110,7 +110,7 @@ fn merge_cmd_preliminary_checks(args: &PangraphMergeArgs, left: &Pangraph, right .header("Suggestion:") })?; - // Circularity is a per-path property, so mixing is structurally fine. It is however most often a + // Circularity is a per-path property, so mixing is structurally fine, but it is most often a // mistake, since `build --circular` applies to all genomes of a graph at once. if circularity(left) != circularity(right) { warn!( diff --git a/packages/pangraph/src/commands/root_args.rs b/packages/pangraph/src/commands/root_args.rs index e6ee0bde..07ef3a1d 100644 --- a/packages/pangraph/src/commands/root_args.rs +++ b/packages/pangraph/src/commands/root_args.rs @@ -187,7 +187,7 @@ mod tests { /// This pins the assignment so that mistake is a test failure. An argument appended after a /// flattened group shows up in one of these sets; an argument added before it does not, so there /// are no false alarms. The escape hatch, if a trailing argument really is needed, is to set - /// `#[clap(help_heading = ...)]` on the argument itself — that wins over the cursor. + /// `#[clap(help_heading = ...)]` on the argument itself, which wins over the cursor. #[rstest] fn test_arguments_are_filed_under_the_expected_help_section() { // `jobs` is declared after the `Verbosity` flatten and used to be swept into it. diff --git a/packages/pangraph/src/pangraph/pangraph.rs b/packages/pangraph/src/pangraph/pangraph.rs index 9b90e1d2..f8761052 100644 --- a/packages/pangraph/src/pangraph/pangraph.rs +++ b/packages/pangraph/src/pangraph/pangraph.rs @@ -234,7 +234,7 @@ impl Pangraph { /// index the maps directly and treat a lookup that fails anyway as an internal error. /// /// Deliberately limited to what a bad file can break. It does *not* check that the graph is - /// semantically coherent — that node positions tile the genome, that edits do not overlap — since + /// semantically coherent (that node positions tile the genome, that edits do not overlap), since /// those are symptoms of a bug in pangraph rather than of a bad input, and are covered by /// [`Self::sanity_check`] in debug builds. Errors are reported as ordinary user-facing errors for /// the same reason: the offending graph came from the user. diff --git a/packages/pangraph/src/pangraph/pangraph_node.rs b/packages/pangraph/src/pangraph/pangraph_node.rs index 55419867..6a9b7207 100644 --- a/packages/pangraph/src/pangraph/pangraph_node.rs +++ b/packages/pangraph/src/pangraph/pangraph_node.rs @@ -38,8 +38,8 @@ impl PangraphNode { /// Creates a node with an explicit id. /// /// Node ids are content-derived, and [`PangraphNode::with_derived_id`] is the one place that - /// derives them. This constructor takes the id it is given, so a caller that has an id already — - /// because the node is being rewritten rather than created — cannot accidentally seed a second, + /// derives them. This constructor takes the id it is given, so a caller that already has an id + /// (because the node is being rewritten rather than created) cannot accidentally seed a second, /// divergent derivation scheme. pub fn new(id: NodeId, block_id: BlockId, path_id: PathId, strand: Strand, position: (usize, usize)) -> Self { Self { diff --git a/packages/pangraph/src/pangraph/reconstruct.rs b/packages/pangraph/src/pangraph/reconstruct.rs index 236f81d0..3ca4caa9 100644 --- a/packages/pangraph/src/pangraph/reconstruct.rs +++ b/packages/pangraph/src/pangraph/reconstruct.rs @@ -279,8 +279,8 @@ pub fn verify_graph_sequences( /// Used to verify a merged graph against the graphs it was built from. Both sides of every /// comparison are reconstructed on demand and dropped again, so this holds two genomes at a time /// instead of the whole sequence content of `sources`. The saving is proportional to the total -/// genome length and modest in practice — peak usage during a merge is dominated by the graphs -/// themselves — but it keeps verification consistent with `reconstruct --verify`, which streams for +/// genome length and modest in practice (peak usage during a merge is dominated by the graphs +/// themselves), but it keeps verification consistent with `reconstruct --verify`, which streams for /// the same reason. /// /// Every genome of `sources` must appear in `graph`, and `graph` must contain nothing else. The @@ -547,7 +547,7 @@ mod tests { assert!(report_to_string(&check_sequence_names(&fastas).unwrap_err()).contains("Duplicate sequence names")); } - /// `> id` — a space between the '>' and the identifier — parses into an empty name with the + /// `> id`, with a space between the '>' and the identifier, parses into an empty name with the /// identifier in the description, so this common header style used to yield a nameless genome. /// The record index is part of the message, since the name cannot point at the offending record. #[rstest] @@ -566,8 +566,8 @@ mod tests { assert!(err.contains('3'), "unexpected error: {err}"); } - /// A header indented with anything other than a plain space — `>\tid` — keeps the whitespace in - /// the name, which is invisible in every place the name is later shown. + /// A header indented with anything other than a plain space, such as `>\tid`, keeps the whitespace + /// in the name, which is invisible in every place the name is later shown. #[rstest] #[case(" a")] #[case("a ")] diff --git a/packages/pangraph/tests/itest_reconstruct.rs b/packages/pangraph/tests/itest_reconstruct.rs index 10134fb3..b837f2f6 100644 --- a/packages/pangraph/tests/itest_reconstruct.rs +++ b/packages/pangraph/tests/itest_reconstruct.rs @@ -239,7 +239,7 @@ mod tests { /// A graph read from a file was not necessarily written by pangraph, so nothing guarantees its /// cross-references resolve. `reconstruct` used to index the node map directly, so a path naming /// a node the graph does not contain aborted the process with a bare "no entry found for key" - /// panic — and in a release build, where `sanity_check` is compiled out, with no indication of + /// panic, and in a release build, where `sanity_check` is compiled out, with no indication of /// which file was at fault. Validation now happens where the file name is still known. #[rstest] fn itest_reconstruct_rejects_malformed_graph_naming_the_file() -> Result<(), Report> {