From 9dee6dcd08cd5b814e8bbebbaa448a6a26601c55 Mon Sep 17 00:00:00 2001 From: Eric Coissac Date: Thu, 27 Aug 2026 06:45:41 +0200 Subject: [PATCH] refactor: decouple tree construction from Newick serialization Refactor phylogenetic algorithms (`neighbor_joining`, `upgma`) to return an explicit `Tree` struct instead of a serialized Newick string. This change makes serialization an explicit step for downstream consumers via the new public `Tree::to_newick()` method, decoupling tree construction from output formatting. The `siblings` module has also been moved to `siblings_old`. --- src/obikmer2/src/cmd/phylo/mod.rs | 7 ++++--- src/obikphylo/src/lib.rs | 2 +- .../src/{siblings => siblings_old}/alignment.rs | 0 .../src/{siblings => siblings_old}/build.rs | 0 .../src/{siblings => siblings_old}/cache.rs | 0 .../src/{siblings => siblings_old}/cardcomp.rs | 0 .../src/{siblings => siblings_old}/cardinality.rs | 0 .../src/{siblings => siblings_old}/distance.rs | 0 .../src/{siblings => siblings_old}/entropy.rs | 0 .../{siblings => siblings_old}/entropy_annex.rs | 0 .../src/{siblings => siblings_old}/family_scan.rs | 0 .../src/{siblings => siblings_old}/helpers.rs | 0 .../src/{siblings => siblings_old}/iter.rs | 0 .../src/{siblings => siblings_old}/mod.rs | 0 .../{siblings => siblings_old}/sankoff_bundle.rs | 0 .../{siblings => siblings_old}/siblingannex.rs | 0 .../src/{siblings => siblings_old}/stats.rs | 0 .../src/{siblings => siblings_old}/subsample.rs | 0 .../src/{siblings => siblings_old}/tests.rs | 0 src/obikphylo/src/tree/mod.rs | 15 +++++++++++---- src/obikphylo/src/tree/nj.rs | 12 ++++++------ src/obikphylo/src/tree/upgma.rs | 10 +++++----- 22 files changed, 27 insertions(+), 19 deletions(-) rename src/obikphylo/src/{siblings => siblings_old}/alignment.rs (100%) rename src/obikphylo/src/{siblings => siblings_old}/build.rs (100%) rename src/obikphylo/src/{siblings => siblings_old}/cache.rs (100%) rename src/obikphylo/src/{siblings => siblings_old}/cardcomp.rs (100%) rename src/obikphylo/src/{siblings => siblings_old}/cardinality.rs (100%) rename src/obikphylo/src/{siblings => siblings_old}/distance.rs (100%) rename src/obikphylo/src/{siblings => siblings_old}/entropy.rs (100%) rename src/obikphylo/src/{siblings => siblings_old}/entropy_annex.rs (100%) rename src/obikphylo/src/{siblings => siblings_old}/family_scan.rs (100%) rename src/obikphylo/src/{siblings => siblings_old}/helpers.rs (100%) rename src/obikphylo/src/{siblings => siblings_old}/iter.rs (100%) rename src/obikphylo/src/{siblings => siblings_old}/mod.rs (100%) rename src/obikphylo/src/{siblings => siblings_old}/sankoff_bundle.rs (100%) rename src/obikphylo/src/{siblings => siblings_old}/siblingannex.rs (100%) rename src/obikphylo/src/{siblings => siblings_old}/stats.rs (100%) rename src/obikphylo/src/{siblings => siblings_old}/subsample.rs (100%) rename src/obikphylo/src/{siblings => siblings_old}/tests.rs (100%) diff --git a/src/obikmer2/src/cmd/phylo/mod.rs b/src/obikmer2/src/cmd/phylo/mod.rs index 2d374a78..49615970 100644 --- a/src/obikmer2/src/cmd/phylo/mod.rs +++ b/src/obikmer2/src/cmd/phylo/mod.rs @@ -5,7 +5,7 @@ use std::sync::Arc; use obikidxcache::index_cache::IndexCache; use obikindex::KmerIndex; -use obikphylo::{Metrics, neighbor_joining_newick, upgma_newick}; +use obikphylo::{Metrics, neighbor_joining, upgma}; use obisys::{Reporter, Stage}; use tracing::info; @@ -103,10 +103,11 @@ pub fn run(args: PhyloArgs) { // ── NJ tree ──────────────────────────────────────────────────────────────── if args.nj { - let newick = neighbor_joining_newick(&result.matrix, &labels).unwrap_or_else(|e| { + let tree = neighbor_joining(&result.matrix, &labels).unwrap_or_else(|e| { eprintln!("error computing NJ tree: {e}"); std::process::exit(1); }); + let newick = tree.to_newick(); let path = args.output.as_ref() .map(|p| format!("{}_nj.nwk", p.display())) .unwrap_or_else(|| "nj.nwk".into()); @@ -119,7 +120,7 @@ pub fn run(args: PhyloArgs) { // ── UPGMA tree ─────────────────────────────────────────────────────────────── if args.upgma { - let newick = upgma_newick(&result.matrix, &labels); + let newick = upgma(&result.matrix, &labels).to_newick(); let path = args.output.as_ref() .map(|p| format!("{}_upgma.nwk", p.display())) .unwrap_or_else(|| "upgma.nwk".into()); diff --git a/src/obikphylo/src/lib.rs b/src/obikphylo/src/lib.rs index 2d2e5cce..d6611cfc 100644 --- a/src/obikphylo/src/lib.rs +++ b/src/obikphylo/src/lib.rs @@ -19,4 +19,4 @@ mod tree; // pub mod siblings; // temporarily disconnected — see module doc above. pub use distance::{DistanceMetric, DistanceOutput, Metrics}; -pub use tree::{neighbor_joining_newick, upgma_newick}; +pub use tree::{Tree, neighbor_joining, upgma}; diff --git a/src/obikphylo/src/siblings/alignment.rs b/src/obikphylo/src/siblings_old/alignment.rs similarity index 100% rename from src/obikphylo/src/siblings/alignment.rs rename to src/obikphylo/src/siblings_old/alignment.rs diff --git a/src/obikphylo/src/siblings/build.rs b/src/obikphylo/src/siblings_old/build.rs similarity index 100% rename from src/obikphylo/src/siblings/build.rs rename to src/obikphylo/src/siblings_old/build.rs diff --git a/src/obikphylo/src/siblings/cache.rs b/src/obikphylo/src/siblings_old/cache.rs similarity index 100% rename from src/obikphylo/src/siblings/cache.rs rename to src/obikphylo/src/siblings_old/cache.rs diff --git a/src/obikphylo/src/siblings/cardcomp.rs b/src/obikphylo/src/siblings_old/cardcomp.rs similarity index 100% rename from src/obikphylo/src/siblings/cardcomp.rs rename to src/obikphylo/src/siblings_old/cardcomp.rs diff --git a/src/obikphylo/src/siblings/cardinality.rs b/src/obikphylo/src/siblings_old/cardinality.rs similarity index 100% rename from src/obikphylo/src/siblings/cardinality.rs rename to src/obikphylo/src/siblings_old/cardinality.rs diff --git a/src/obikphylo/src/siblings/distance.rs b/src/obikphylo/src/siblings_old/distance.rs similarity index 100% rename from src/obikphylo/src/siblings/distance.rs rename to src/obikphylo/src/siblings_old/distance.rs diff --git a/src/obikphylo/src/siblings/entropy.rs b/src/obikphylo/src/siblings_old/entropy.rs similarity index 100% rename from src/obikphylo/src/siblings/entropy.rs rename to src/obikphylo/src/siblings_old/entropy.rs diff --git a/src/obikphylo/src/siblings/entropy_annex.rs b/src/obikphylo/src/siblings_old/entropy_annex.rs similarity index 100% rename from src/obikphylo/src/siblings/entropy_annex.rs rename to src/obikphylo/src/siblings_old/entropy_annex.rs diff --git a/src/obikphylo/src/siblings/family_scan.rs b/src/obikphylo/src/siblings_old/family_scan.rs similarity index 100% rename from src/obikphylo/src/siblings/family_scan.rs rename to src/obikphylo/src/siblings_old/family_scan.rs diff --git a/src/obikphylo/src/siblings/helpers.rs b/src/obikphylo/src/siblings_old/helpers.rs similarity index 100% rename from src/obikphylo/src/siblings/helpers.rs rename to src/obikphylo/src/siblings_old/helpers.rs diff --git a/src/obikphylo/src/siblings/iter.rs b/src/obikphylo/src/siblings_old/iter.rs similarity index 100% rename from src/obikphylo/src/siblings/iter.rs rename to src/obikphylo/src/siblings_old/iter.rs diff --git a/src/obikphylo/src/siblings/mod.rs b/src/obikphylo/src/siblings_old/mod.rs similarity index 100% rename from src/obikphylo/src/siblings/mod.rs rename to src/obikphylo/src/siblings_old/mod.rs diff --git a/src/obikphylo/src/siblings/sankoff_bundle.rs b/src/obikphylo/src/siblings_old/sankoff_bundle.rs similarity index 100% rename from src/obikphylo/src/siblings/sankoff_bundle.rs rename to src/obikphylo/src/siblings_old/sankoff_bundle.rs diff --git a/src/obikphylo/src/siblings/siblingannex.rs b/src/obikphylo/src/siblings_old/siblingannex.rs similarity index 100% rename from src/obikphylo/src/siblings/siblingannex.rs rename to src/obikphylo/src/siblings_old/siblingannex.rs diff --git a/src/obikphylo/src/siblings/stats.rs b/src/obikphylo/src/siblings_old/stats.rs similarity index 100% rename from src/obikphylo/src/siblings/stats.rs rename to src/obikphylo/src/siblings_old/stats.rs diff --git a/src/obikphylo/src/siblings/subsample.rs b/src/obikphylo/src/siblings_old/subsample.rs similarity index 100% rename from src/obikphylo/src/siblings/subsample.rs rename to src/obikphylo/src/siblings_old/subsample.rs diff --git a/src/obikphylo/src/siblings/tests.rs b/src/obikphylo/src/siblings_old/tests.rs similarity index 100% rename from src/obikphylo/src/siblings/tests.rs rename to src/obikphylo/src/siblings_old/tests.rs diff --git a/src/obikphylo/src/tree/mod.rs b/src/obikphylo/src/tree/mod.rs index 4115c5fe..3b7e1e4d 100644 --- a/src/obikphylo/src/tree/mod.rs +++ b/src/obikphylo/src/tree/mod.rs @@ -3,13 +3,20 @@ //! shared Newick serialisation, instead of each algorithm hand-rolling its //! own Newick writer. No reader — nothing in this crate ever needs to parse //! a Newick string back in, so there is no `from_newick`. +//! +//! Two independent axes, deliberately kept from ever multiplying against +//! each other: [`nj::neighbor_joining`]/[`upgma::upgma`] each only know how +//! to build a [`Tree`] from their own algorithm's result — neither knows +//! Newick exists. [`Tree::to_newick`] only knows how to walk an already-built +//! `Tree` — it doesn't know NJ or UPGMA exist. A new algorithm or a new +//! output format each plug in on their own side, at zero cost to the other. mod newick; mod nj; mod upgma; -pub use nj::neighbor_joining_newick; -pub use upgma::upgma_newick; +pub use nj::neighbor_joining; +pub use upgma::upgma; #[derive(Debug, Clone, Copy, PartialEq, Eq)] struct NodeId(usize); @@ -28,7 +35,7 @@ struct Node { /// algorithm (UPGMA's successive merges, NJ's already-built graph) needs: /// a node's parent is only known once *it* already exists. #[derive(Debug, Default)] -struct Tree { +pub struct Tree { nodes: Vec, root: Option, } @@ -75,7 +82,7 @@ impl Tree { /// Serialise to Newick (terminated with `;`). Panics if /// [`set_root`](Tree::set_root) was never called — a tree with no /// designated root is a builder bug, not a recoverable input. - fn to_newick(&self) -> String { + pub fn to_newick(&self) -> String { let root = self.root.expect("Tree::to_newick: root not set"); let mut out = newick::write_node(self, root); out.push(';'); diff --git a/src/obikphylo/src/tree/nj.rs b/src/obikphylo/src/tree/nj.rs index 8655afef..bec4d90c 100644 --- a/src/obikphylo/src/tree/nj.rs +++ b/src/obikphylo/src/tree/nj.rs @@ -1,7 +1,7 @@ //! Neighbor-Joining, via `speedytree` — converts its own tree type -//! (`petgraph::graph::UnGraph`) into this crate's [`super::Tree`] -//! so its Newick output goes through the same writer as every other -//! algorithm here. +//! (`petgraph::graph::UnGraph`) into this crate's [`super::Tree`]. +//! Knows nothing about Newick or any other output format — that's +//! [`Tree::to_newick`]'s job, not this algorithm's. use ndarray::Array2; use obikindex::{OKIError, OKIResult}; @@ -11,8 +11,8 @@ use speedytree::{DistanceMatrix, Hybrid, NeighborJoiningSolver}; use super::{NodeId, Tree}; /// Compute a Neighbor-Joining tree from a symmetric `n×n` distance `matrix` -/// (genomes in `labels` order), returning it as a Newick string. -pub fn neighbor_joining_newick(matrix: &Array2, labels: &[String]) -> OKIResult { +/// (genomes in `labels` order). +pub fn neighbor_joining(matrix: &Array2, labels: &[String]) -> OKIResult { let n = labels.len(); let rows: Vec> = (0..n).map(|i| (0..n).map(|j| matrix[[i, j]]).collect()).collect(); let dm = DistanceMatrix::build(rows, labels.to_vec()) @@ -33,7 +33,7 @@ pub fn neighbor_joining_newick(matrix: &Array2, labels: &[String]) -> OKIRe let mut tree = Tree::new(); let root_id = convert(&graph, root, root, &mut tree); tree.set_root(root_id); - Ok(tree.to_newick()) + Ok(tree) } /// Recursively convert `node` (and everything below it, i.e. every diff --git a/src/obikphylo/src/tree/upgma.rs b/src/obikphylo/src/tree/upgma.rs index d60bf27f..e908b3cc 100644 --- a/src/obikphylo/src/tree/upgma.rs +++ b/src/obikphylo/src/tree/upgma.rs @@ -1,7 +1,7 @@ //! UPGMA (average-linkage hierarchical clustering), via `kodama` — converts //! its dendrogram (a flat list of merge steps) into this crate's -//! [`super::Tree`], bottom-up, so its Newick output goes through the same -//! writer as every other algorithm here. +//! [`super::Tree`], bottom-up. Knows nothing about Newick or any other +//! output format — that's [`Tree::to_newick`]'s job, not this algorithm's. use kodama::{Method, linkage}; use ndarray::Array2; @@ -9,8 +9,8 @@ use ndarray::Array2; use super::Tree; /// Compute a UPGMA tree from a symmetric `n×n` distance `matrix` (genomes in -/// `labels` order), returning it as a Newick string. -pub fn upgma_newick(matrix: &Array2, labels: &[String]) -> String { +/// `labels` order). +pub fn upgma(matrix: &Array2, labels: &[String]) -> Tree { let n = labels.len(); let mut condensed: Vec = Vec::with_capacity(n * (n - 1) / 2); for i in 0..n { @@ -47,5 +47,5 @@ pub fn upgma_newick(matrix: &Array2, labels: &[String]) -> String { } tree.set_root(root.expect("at least one merge step for n >= 2 genomes")); - tree.to_newick() + tree }