From 829af1461c021ed8d3a04a2a5cf068840499b448 Mon Sep 17 00:00:00 2001 From: Mike German Date: Sun, 12 Jul 2026 12:29:05 -0400 Subject: [PATCH 01/13] fix: reassigning Node.edge no longer detaches node from its parent Node._set_edge() removed the node from its parent's _child_nodes list as a side effect of swapping in a new Edge object, without re-adding it anywhere or clearing _parent_node. Since a node's tail_node is derived from head_node._parent_node (not stored independently), this detachment had no bookkeeping purpose and only corrupted the tree: after 'node.edge = new_edge', the node vanished from its parent's children while node.parent_node still pointed at the old parent. Signed-off-by: Mike German --- src/dendropy/datamodel/treemodel/_node.py | 5 ----- .../test_datamodel_tree_node_fundamentals.py | 18 ++++++++++++++++++ 2 files changed, 18 insertions(+), 5 deletions(-) diff --git a/src/dendropy/datamodel/treemodel/_node.py b/src/dendropy/datamodel/treemodel/_node.py index 22336b829..a1c8b8636 100644 --- a/src/dendropy/datamodel/treemodel/_node.py +++ b/src/dendropy/datamodel/treemodel/_node.py @@ -979,11 +979,6 @@ def _set_edge(self, new_edge): # raise ValueError("A Node cannot have 'None' for an edge") if new_edge is self._edge: return - if self._parent_node is not None: - try: - self._parent_node._child_nodes.remove(self) - except ValueError: - pass ## Minimal management self._edge = new_edge diff --git a/tests/unittests/test_datamodel_tree_node_fundamentals.py b/tests/unittests/test_datamodel_tree_node_fundamentals.py index 568551fa8..1ad4e8d74 100644 --- a/tests/unittests/test_datamodel_tree_node_fundamentals.py +++ b/tests/unittests/test_datamodel_tree_node_fundamentals.py @@ -322,5 +322,23 @@ def test_edge_head_node_setting(self): self.assertIs(node.edge, edge2) self.assertIs(node.edge.head_node, node) + def test_edge_setting_does_not_corrupt_parent_child_bookkeeping(self): + # Reassigning a node's ``edge`` is unrelated to the node's position + # in the tree, and should not affect the parent's list of children + # or the node's own parent reference. + parent = dendropy.Node(label="parent") + child1 = dendropy.Node(label="child1") + child2 = dendropy.Node(label="child2") + parent.set_child_nodes([child1, child2]) + self.assertEqual(len(parent.child_nodes()), 2) + self.assertIn(child1, parent.child_nodes()) + self.assertIs(child1.parent_node, parent) + + child1.edge = dendropy.Edge(length=5.0) + + self.assertEqual(len(parent.child_nodes()), 2) + self.assertIn(child1, parent.child_nodes()) + self.assertIs(child1.parent_node, parent) + if __name__ == "__main__": unittest.main() From 6120a6870df58aa98a4aa80dbe19395c9bf548fa Mon Sep 17 00:00:00 2001 From: Mike German Date: Sun, 12 Jul 2026 12:33:11 -0400 Subject: [PATCH 02/13] fix: summarize_node_ages_on_tree honors is_bipartitions_updated TreeSummarizer.summarize_node_ages_on_tree() checked 'if is_bipartitions_updated:' before re-encoding bipartitions, the opposite of every sibling summarization method in this module (which all skip the (deprecated) re-encode when the caller has already updated bipartitions). Passing is_bipartitions_updated=True therefore triggered an unwanted call to the deprecated Tree.encode_splits() instead of skipping it. Signed-off-by: Mike German --- src/dendropy/calculate/treesum.py | 4 +- .../test_tree_summarization_and_consensus.py | 60 +++++++++++++++++++ 2 files changed, 62 insertions(+), 2 deletions(-) diff --git a/src/dendropy/calculate/treesum.py b/src/dendropy/calculate/treesum.py index c00841c00..0d9b27992 100644 --- a/src/dendropy/calculate/treesum.py +++ b/src/dendropy/calculate/treesum.py @@ -242,8 +242,8 @@ def summarize_node_ages_on_tree(self, """ if summarization_fn is None: summarization_fn = lambda x: float(sum(x))/len(x) - if is_bipartitions_updated: - tree.encode_splits() + if not is_bipartitions_updated: + tree.encode_bipartitions() #'height', #'height_median', #'height_95hpd', diff --git a/tests/unittests/test_tree_summarization_and_consensus.py b/tests/unittests/test_tree_summarization_and_consensus.py index ec57dcb0e..4e04b8531 100644 --- a/tests/unittests/test_tree_summarization_and_consensus.py +++ b/tests/unittests/test_tree_summarization_and_consensus.py @@ -26,8 +26,10 @@ import dendropy import random import itertools +import warnings from dendropy.calculate import treecompare from dendropy.calculate import statistics +from dendropy.calculate import treesum import os import sys sys.path.insert(0, os.path.dirname(__file__)) @@ -218,6 +220,64 @@ def testMeanNodeAgeSummarizationOnMCCT(self): obs_edge = target_tree.bipartition_edge_map[exp_bipartition] self.assertAlmostEqual(obs_edge.head_node.age, exp_edge.head_node.age) +class TestSummarizeNodeAgesOnTree(unittest.TestCase): + """ + Regression tests for TreeSummarizer.summarize_node_ages_on_tree(). + """ + + def setUp(self): + self.taxon_namespace = dendropy.TaxonNamespace(["A", "B", "C"]) + support_newicks = [ + "(A:1,(B:1,C:1):1):0;", + "(A:2,(B:1,C:2):1):0;", + "(A:1,(B:2,C:1):2):0;", + ] + self.support_trees = dendropy.TreeList(taxon_namespace=self.taxon_namespace) + for newick in support_newicks: + tree = dendropy.Tree.get( + data=newick, + schema="newick", + taxon_namespace=self.taxon_namespace) + tree.encode_bipartitions() + self.support_trees.append(tree) + self.split_distribution = dendropy.SplitDistribution( + taxon_namespace=self.taxon_namespace) + for tree in self.support_trees: + self.split_distribution.count_splits_on_tree(tree) + + def _get_target_tree(self): + target_tree = dendropy.Tree.get( + data="(A:1,(B:1,C:1):1):0;", + schema="newick", + taxon_namespace=self.taxon_namespace) + target_tree.encode_bipartitions() + return target_tree + + def test_is_bipartitions_updated_true_skips_reencoding(self): + # 'is_bipartitions_updated=True' used to trigger a *re*-encode via + # the deprecated 'encode_splits()' (inverted vs. every sibling + # summarization method in this module, which all skip re-encoding + # when the caller asserts bipartitions are already current). + target_tree = self._get_target_tree() + summarizer = treesum.TreeSummarizer() + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter("always") + summarizer.summarize_node_ages_on_tree( + tree=target_tree, + split_distribution=self.split_distribution, + set_edge_lengths=False, + collapse_negative_edges=False, + is_bipartitions_updated=True, + ) + deprecation_warnings = [ + w for w in caught + if "encode_splits" in str(w.message)] + self.assertEqual( + len(deprecation_warnings), 0, + "summarize_node_ages_on_tree(is_bipartitions_updated=True) " + "should not re-encode bipartitions via the deprecated " + "encode_splits()") + class TestTopologyCounter(dendropytest.ExtendedTestCase): def get_regime(self, From 76983ad023703f83e2b0782a10be1c509699b17a Mon Sep 17 00:00:00 2001 From: Mike German Date: Sun, 12 Jul 2026 12:35:23 -0400 Subject: [PATCH 03/13] fix: collapse_negative_edges no longer crashes in summarize_node_ages_on_tree The 'force parent at least as old as its oldest child' step ran inside the same preorder pass used to assign ages, so it compared a node's age against children that the preorder traversal had not visited yet (still None), raising a TypeError on essentially any tree. Move the adjustment to its own postorder pass, run after all ages are assigned. Signed-off-by: Mike German --- src/dendropy/calculate/treesum.py | 8 ++++++-- .../test_tree_summarization_and_consensus.py | 17 +++++++++++++++++ 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/src/dendropy/calculate/treesum.py b/src/dendropy/calculate/treesum.py index 0d9b27992..8eb8a1e3c 100644 --- a/src/dendropy/calculate/treesum.py +++ b/src/dendropy/calculate/treesum.py @@ -263,8 +263,12 @@ def summarize_node_ages_on_tree(self, nd.age = nd.parent_node.age else: nd.age = 0 - ## force parent nodes to be at least as old as their oldest child - if collapse_negative_edges: + if collapse_negative_edges: + ## force parent nodes to be at least as old as their oldest + ## child; this requires a postorder pass, since the preorder + ## pass above visits parents before the children whose ages + ## it needs to compare against + for nd in tree.postorder_node_iter(): for child in nd.child_nodes(): if child.age > nd.age: nd.age = child.age diff --git a/tests/unittests/test_tree_summarization_and_consensus.py b/tests/unittests/test_tree_summarization_and_consensus.py index 4e04b8531..2509bb04f 100644 --- a/tests/unittests/test_tree_summarization_and_consensus.py +++ b/tests/unittests/test_tree_summarization_and_consensus.py @@ -278,6 +278,23 @@ def test_is_bipartitions_updated_true_skips_reencoding(self): "should not re-encode bipartitions via the deprecated " "encode_splits()") + def test_collapse_negative_edges_does_not_crash(self): + # 'collapse_negative_edges=True' used to compare a not-yet-visited + # child's 'age' (still None, since ages are assigned in preorder) + # against its parent's, raising a TypeError. + target_tree = self._get_target_tree() + summarizer = treesum.TreeSummarizer() + result_tree = summarizer.summarize_node_ages_on_tree( + tree=target_tree, + split_distribution=self.split_distribution, + set_edge_lengths=True, + collapse_negative_edges=True, + is_bipartitions_updated=True, + ) + for nd in result_tree.preorder_node_iter(): + if nd.parent_node is not None: + self.assertGreaterEqual(nd.parent_node.age, nd.age) + class TestTopologyCounter(dendropytest.ExtendedTestCase): def get_regime(self, From 2ad80e6363ddbc1e0fba749d7023a2b63c6ef931 Mon Sep 17 00:00:00 2001 From: Mike German Date: Sun, 12 Jul 2026 12:37:59 -0400 Subject: [PATCH 04/13] fix: unweighted SES statistics now actually randomize the null model _calculate_standardized_effect_size() passed is_shuffle_phylogenetic_distances instead of is_shuffle_phylogenetic_path_steps to shuffle_taxa(). For is_weighted_edge_distances=False, this meant both shuffle flags were False, so the null-model replicates were never randomized at all, silently producing a null_model_sd of 0 for standardized effect size calculations on unweighted (edge-count) distances. Signed-off-by: Mike German --- .../calculate/phylogeneticdistance.py | 2 +- .../test_phylogenetic_distance_matrix.py | 49 +++++++++++++++++++ 2 files changed, 50 insertions(+), 1 deletion(-) diff --git a/src/dendropy/calculate/phylogeneticdistance.py b/src/dendropy/calculate/phylogeneticdistance.py index dc7c6273c..0fa166578 100644 --- a/src/dendropy/calculate/phylogeneticdistance.py +++ b/src/dendropy/calculate/phylogeneticdistance.py @@ -1362,7 +1362,7 @@ def _calculate_standardized_effect_size(self, for rep_idx in range(num_randomization_replicates): null_model_matrix.shuffle_taxa( is_shuffle_phylogenetic_distances=is_shuffle_phylogenetic_distances, - is_shuffle_phylogenetic_path_steps=is_shuffle_phylogenetic_distances, + is_shuffle_phylogenetic_path_steps=is_shuffle_phylogenetic_path_steps, is_shuffle_mrca=False, rng=rng) for comparison_regime_idx, comparison_regime in enumerate(comparison_regimes): diff --git a/tests/unittests/test_phylogenetic_distance_matrix.py b/tests/unittests/test_phylogenetic_distance_matrix.py index 969a88beb..f1b938ad8 100644 --- a/tests/unittests/test_phylogenetic_distance_matrix.py +++ b/tests/unittests/test_phylogenetic_distance_matrix.py @@ -339,6 +339,55 @@ def test_shuffle(self): self.assertEqual(pdc1, pdc2) self.assertNotEqual(pdc0, pdc1) +class PhylogeneticDistanceMatrixStandardizedEffectSizeShuffleTest(unittest.TestCase): + """ + Regression test for _calculate_standardized_effect_size(): for the + unweighted-edge-distance case, the null model randomization must + actually shuffle '_taxon_phylogenetic_path_steps' (the matrix that + unweighted statistics read from), not silently leave it untouched. + """ + + def setUp(self): + self.tree = dendropy.Tree.get_from_path( + src=pathmap.tree_source_path("community.tree.newick"), + schema="newick", + rooting="force-rooted") + self.pdm = dendropy.PhylogeneticDistanceMatrix.from_tree(self.tree) + taxa = list(self.pdm.taxon_iter()) + filter_fn = lambda taxon: taxon in set(taxa) + self.comparison_regime = list(self.pdm.distinct_taxon_pair_iter(filter_fn=filter_fn)) + + def test_unweighted_null_model_shuffles_path_steps(self): + import random + from dendropy.calculate.phylogeneticdistance import PhylogeneticDistanceMatrix + + observed_kwargs = [] + original_shuffle_taxa = PhylogeneticDistanceMatrix.shuffle_taxa + def recording_shuffle_taxa(self, *args, **kwargs): + observed_kwargs.append(dict(kwargs)) + return original_shuffle_taxa(self, *args, **kwargs) + PhylogeneticDistanceMatrix.shuffle_taxa = recording_shuffle_taxa + try: + self.pdm._calculate_standardized_effect_size( + statisticf_name="_calculate_mean_pairwise_distance", + comparison_regimes=[self.comparison_regime], + is_weighted_edge_distances=False, + is_normalize_by_tree_size=False, + num_randomization_replicates=3, + rng=random.Random(1), + ) + finally: + PhylogeneticDistanceMatrix.shuffle_taxa = original_shuffle_taxa + self.assertTrue(len(observed_kwargs) > 0) + for kwargs in observed_kwargs: + self.assertTrue( + kwargs["is_shuffle_phylogenetic_path_steps"], + "is_weighted_edge_distances=False must shuffle " + "phylogenetic path steps for the null model to be " + "meaningful (unweighted statistics read from " + "'_taxon_phylogenetic_path_steps', not " + "'_taxon_phylogenetic_distances')") + class TreePatristicDistTest(unittest.TestCase): def setUp(self): From a9e041f955c7d2d6c4ba5046218f49840d82ae97 Mon Sep 17 00:00:00 2001 From: Mike German Date: Sun, 12 Jul 2026 12:40:23 -0400 Subject: [PATCH 05/13] fix: simulate_discrete_chars() now honors the root_states argument simulate_discrete_chars() accepts a 'root_states' parameter but passed 'root_states=None' on to DiscreteCharacterEvolver.evolve_states(), silently discarding it and always simulating a random root sequence from the model's stationary distribution instead of the caller's requested states. Signed-off-by: Mike German --- src/dendropy/model/discrete.py | 2 +- tests/unittests/test_discrete.py | 24 ++++++++++++++++++++++++ 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/src/dendropy/model/discrete.py b/src/dendropy/model/discrete.py index a90d3a83a..b93d20cbb 100644 --- a/src/dendropy/model/discrete.py +++ b/src/dendropy/model/discrete.py @@ -496,7 +496,7 @@ def simulate_discrete_chars( tree = seq_evolver.evolve_states( tree=tree_model, seq_len=seq_len, - root_states=None, + root_states=root_states, rng=rng) tree.migrate_taxon_namespace(tree_model.taxon_namespace) if char_matrix is None: diff --git a/tests/unittests/test_discrete.py b/tests/unittests/test_discrete.py index 8fcaa1c14..bfbeb4f53 100644 --- a/tests/unittests/test_discrete.py +++ b/tests/unittests/test_discrete.py @@ -23,6 +23,7 @@ import random import unittest +import dendropy from dendropy.model import discrete from dendropy.simulate import treesim @@ -56,5 +57,28 @@ def test_rng_param(self): ) ) + def test_simulate_discrete_chars_honors_root_states(self): + """ + Regression test: simulate_discrete_chars() accepts a 'root_states' + parameter but was passing 'root_states=None' on to the underlying + evolver, silently discarding the caller's requested root sequence. + """ + tree = dendropy.Tree.get(data="(A:1,B:1):0;", schema="newick") + seq_model = discrete.Jc69() + alphabet_states = list(seq_model.state_alphabet) + seq_len = 8 + root_states = [alphabet_states[i % 4] for i in range(seq_len)] + discrete.simulate_discrete_chars( + seq_len=seq_len, + tree_model=tree, + seq_model=seq_model, + root_states=root_states, + retain_sequences_on_tree=True, + rng=random.Random(1), + ) + root_node = tree.seed_node + root_sequence = root_node.sequences[-1] + self.assertEqual(list(root_sequence), root_states) + if __name__ == "__main__": unittest.main() From 9b250716a344e6d0814cea5ec46e5b5ce3b666fb Mon Sep 17 00:00:00 2001 From: Mike German Date: Sun, 12 Jul 2026 12:44:17 -0400 Subject: [PATCH 06/13] fix: CharacterMatrix.concatenate() error message substitutes matrix index The unequal-length-sequences error used a '%d' printf-style placeholder but called '.format()' on the string (which only substitutes '{}' placeholders), so the raised ValueError always showed the literal, unhelpful '%d' instead of the offending matrix's index. Signed-off-by: Mike German --- src/dendropy/datamodel/charmatrixmodel.py | 2 +- tests/unittests/test_datamodel_charmatrix.py | 16 ++++++++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/src/dendropy/datamodel/charmatrixmodel.py b/src/dendropy/datamodel/charmatrixmodel.py index 4a4be328b..dc03c1720 100644 --- a/src/dendropy/datamodel/charmatrixmodel.py +++ b/src/dendropy/datamodel/charmatrixmodel.py @@ -628,7 +628,7 @@ def concatenate(cls, char_matrices): v1 = len(cm[0]) for t, s in cm.items(): if len(s) != v1: - raise ValueError("Unequal length sequences in character matrix %d".format(cidx+1)) + raise ValueError("Unequal length sequences in character matrix {}".format(cidx+1)) concatenated_chars.extend_matrix(cm) if cm.label is None: new_label = "locus%03d" % cidx diff --git a/tests/unittests/test_datamodel_charmatrix.py b/tests/unittests/test_datamodel_charmatrix.py index 6fd141324..5268046f3 100644 --- a/tests/unittests/test_datamodel_charmatrix.py +++ b/tests/unittests/test_datamodel_charmatrix.py @@ -439,6 +439,22 @@ def test_extend_matrix(self): self.verify_sequence_equal(c1[tns[1]], [2, 2, 2, 3, 3, 3]) self.verify_sequence_equal(c1[tns[2]], [4, 4, 4]) +class CharacterMatrixConcatenateTest(dendropytest.ExtendedTestCase): + + def test_unequal_length_sequence_error_message(self): + # Regression test: the error message used the '%d' printf-style + # placeholder but applied '.format()' to the string, so the + # placeholder was never substituted and the matrix index was + # missing from the raised message. + tns = get_taxon_namespace(2) + cm1 = charmatrixmodel.CharacterMatrix(taxon_namespace=tns) + cm1[tns[0]] = [1, 1, 1, 1] + cm1[tns[1]] = [1, 1] + with self.assertRaises(ValueError) as ctx: + charmatrixmodel.CharacterMatrix.concatenate([cm1]) + self.assertIn("1", str(ctx.exception)) + self.assertNotIn("%d", str(ctx.exception)) + class CharacterMatrixTaxonManagement(dendropytest.ExtendedTestCase): def test_assign_taxon_namespace(self): From edd646e4ffd692261bcf78ce6185c894c6c3edf0 Mon Sep 17 00:00:00 2001 From: Mike German Date: Sun, 12 Jul 2026 12:45:28 -0400 Subject: [PATCH 07/13] fix: CharacterMatrix.concatenate() no longer hangs on duplicate labels The character-subset label de-duplication loop computed a new candidate into 'label' but checked/looped on 'cs_label', which was never reassigned. Any label collision (e.g. concatenating two matrices with the same or default label) caused an infinite loop. Signed-off-by: Mike German --- src/dendropy/datamodel/charmatrixmodel.py | 2 +- tests/unittests/test_datamodel_charmatrix.py | 20 ++++++++++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/src/dendropy/datamodel/charmatrixmodel.py b/src/dendropy/datamodel/charmatrixmodel.py index dc03c1720..d05735923 100644 --- a/src/dendropy/datamodel/charmatrixmodel.py +++ b/src/dendropy/datamodel/charmatrixmodel.py @@ -637,7 +637,7 @@ def concatenate(cls, char_matrices): cs_label = new_label i = 2 while cs_label in concatenated_chars.character_subsets: - label = "%s_%03d" % (new_label, i) + cs_label = "%s_%03d" % (new_label, i) i += 1 character_indices = range(pos_start, pos_start + cm.vector_size) pos_start += cm.vector_size diff --git a/tests/unittests/test_datamodel_charmatrix.py b/tests/unittests/test_datamodel_charmatrix.py index 5268046f3..0c8acb6d1 100644 --- a/tests/unittests/test_datamodel_charmatrix.py +++ b/tests/unittests/test_datamodel_charmatrix.py @@ -455,6 +455,26 @@ def test_unequal_length_sequence_error_message(self): self.assertIn("1", str(ctx.exception)) self.assertNotIn("%d", str(ctx.exception)) + def test_concatenate_does_not_hang_on_duplicate_labels(self): + # Regression test: when two matrices being concatenated share the + # same (or no) label, the de-duplication loop computed a new + # candidate label into 'label' but checked/looped on 'cs_label', + # which never changed -- an infinite loop on any label collision. + tns = get_taxon_namespace(2) + cm1 = charmatrixmodel.CharacterMatrix(taxon_namespace=tns) + cm1.label = "dupe" + cm1[tns[0]] = [1, 1] + cm1[tns[1]] = [1, 1] + cm2 = charmatrixmodel.CharacterMatrix(taxon_namespace=tns) + cm2.label = "dupe" + cm2[tns[0]] = [2, 2] + cm2[tns[1]] = [2, 2] + concatenated = charmatrixmodel.CharacterMatrix.concatenate([cm1, cm2]) + self.assertEqual(len(concatenated.character_subsets), 2) + labels = set(concatenated.character_subsets.keys()) + self.assertIn("dupe", labels) + self.assertEqual(len(labels), 2) + class CharacterMatrixTaxonManagement(dendropytest.ExtendedTestCase): def test_assign_taxon_namespace(self): From 6e21ef659f0593cc1ca3c2e71743c39bc33253ba Mon Sep 17 00:00:00 2001 From: Mike German Date: Sun, 12 Jul 2026 12:47:24 -0400 Subject: [PATCH 08/13] fix: DiscreteCharacterMatrix.append_taxon_sequence() no longer crashes append_taxon_sequence() referenced 'self.default_symbol_state_map', an attribute that does not exist anywhere on CharacterMatrix or its subclasses, so any call with a non-empty 'state_symbols' argument raised an AttributeError. Use the existing 'default_state_alphabet.full_symbol_state_map' lookup instead. Signed-off-by: Mike German --- src/dendropy/datamodel/charmatrixmodel.py | 2 +- tests/unittests/test_datamodel_charmatrix.py | 15 +++++++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/src/dendropy/datamodel/charmatrixmodel.py b/src/dendropy/datamodel/charmatrixmodel.py index d05735923..8be381821 100644 --- a/src/dendropy/datamodel/charmatrixmodel.py +++ b/src/dendropy/datamodel/charmatrixmodel.py @@ -1759,7 +1759,7 @@ def append_taxon_sequence(self, taxon, state_symbols): symbol = value else: symbol = str(value) - self[taxon].append(self.default_symbol_state_map[symbol]) + self[taxon].append(self.default_state_alphabet.full_symbol_state_map[symbol]) def remap_to_state_alphabet_by_symbol(self, state_alphabet, diff --git a/tests/unittests/test_datamodel_charmatrix.py b/tests/unittests/test_datamodel_charmatrix.py index 0c8acb6d1..dac7805a7 100644 --- a/tests/unittests/test_datamodel_charmatrix.py +++ b/tests/unittests/test_datamodel_charmatrix.py @@ -475,6 +475,21 @@ def test_concatenate_does_not_hang_on_duplicate_labels(self): self.assertIn("dupe", labels) self.assertEqual(len(labels), 2) +class CharacterMatrixAppendTaxonSequenceTest(dendropytest.ExtendedTestCase): + + def test_append_taxon_sequence_with_symbols(self): + # Regression test: 'default_symbol_state_map' does not exist on + # DiscreteCharacterMatrix (or any subclass); any non-empty call + # raised an AttributeError. + tns = get_taxon_namespace(1) + cm = dendropy.DnaCharacterMatrix(taxon_namespace=tns) + taxon = tns[0] + cm.append_taxon_sequence(taxon, "ACGT") + self.assertEqual(len(cm[taxon]), 4) + self.assertEqual( + [s.symbol for s in cm[taxon]], + ["A", "C", "G", "T"]) + class CharacterMatrixTaxonManagement(dendropytest.ExtendedTestCase): def test_assign_taxon_namespace(self): From ff87b3f238c0fdddd17c28637d21fda533bfa492 Mon Sep 17 00:00:00 2001 From: Mike German Date: Sun, 12 Jul 2026 12:49:40 -0400 Subject: [PATCH 09/13] fix: NormalizedBitmaskDict no longer corrupts OrderedDict iteration __setitem__() and __delitem__() called dict.__setitem__()/ dict.__delitem__() directly, bypassing OrderedDict's internal bookkeeping. Reads (__getitem__, __contains__, get) were unaffected, but iteration (items()/keys()/values()/__iter__(), and thus len() staying correct while iteration went empty) was silently broken after any mutation. Signed-off-by: Mike German --- src/dendropy/utility/container.py | 4 ++-- .../test_container_normalized_bitmask_dict.py | 16 ++++++++++++++-- 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/src/dendropy/utility/container.py b/src/dendropy/utility/container.py index 876ef7249..582924fc0 100644 --- a/src/dendropy/utility/container.py +++ b/src/dendropy/utility/container.py @@ -283,7 +283,7 @@ def __deepcopy__(self, memo): def normalize_key_and_assign_value(self, key, value): "*Almost* like __setitem__(), but returns value of normalized key to calling code." normalized_key = self.normalize_key(key) - dict.__setitem__(self, normalized_key, value) + collections.OrderedDict.__setitem__(self, normalized_key, value) return normalized_key def normalize_key(self, key): @@ -301,7 +301,7 @@ def __getitem__(self, key): def __delitem__(self, key): "Remove item with normalized key." key = self.normalize_key(key) - dict.__delitem__(self, key) + collections.OrderedDict.__delitem__(self, key) def __contains__(self, key): "Returns true if has normalized key." diff --git a/tests/unittests/test_container_normalized_bitmask_dict.py b/tests/unittests/test_container_normalized_bitmask_dict.py index 8cc777c8f..32e08a72e 100644 --- a/tests/unittests/test_container_normalized_bitmask_dict.py +++ b/tests/unittests/test_container_normalized_bitmask_dict.py @@ -45,8 +45,14 @@ def runTest(self): self.assertIn(s[1][0], d) self.assertEqual(d[s[0][0]], d[s[1][0]]) - for k, v in d.items(): - pass + # Regression test: NormalizedBitmaskDict.__setitem__() used to call + # dict.__setitem__() directly, bypassing OrderedDict's internal + # bookkeeping. This left len()/__repr__() correct but silently + # broke iteration (.items()/.keys()/.values()/__iter__()), which + # would all report zero entries despite the dict being non-empty. + seen_keys = set(k for k, v in d.items()) + self.assertEqual(len(seen_keys), len(splits)) + self.assertEqual(len(d), len(splits)) del d[splits[0][0][0]] del d[splits[1][1][0]] @@ -55,5 +61,11 @@ def runTest(self): self.assertNotIn(splits[1][0][0], d) self.assertNotIn(splits[1][1][0], d) + # Regression test: __delitem__() had the same dict-vs-OrderedDict + # bypass problem; confirm iteration still reflects the deletions. + seen_keys_after_delete = set(k for k, v in d.items()) + self.assertEqual(len(seen_keys_after_delete), len(splits) - 2) + self.assertEqual(len(d), len(splits) - 2) + if __name__ == "__main__": unittest.main() From a0cec0fd314c5208e9212e1ef219005f14a67ca8 Mon Sep 17 00:00:00 2001 From: Mike German Date: Sun, 12 Jul 2026 12:51:23 -0400 Subject: [PATCH 10/13] fix: Taxon(other_taxon) now preserves comments Taxon.__init__() unconditionally set 'self.comments = []' after the if/else branch, even for the Taxon-from-Taxon copy path, which had just deep-copied 'comments' (along with the rest of __dict__) from the source Taxon a few lines earlier. The unconditional reset silently discarded it, so Taxon(other_taxon), copy.deepcopy(), and .clone() all dropped comments on the copy. Signed-off-by: Mike German --- src/dendropy/datamodel/taxonmodel.py | 2 +- tests/unittests/test_datamodel_taxon.py | 11 +++++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/src/dendropy/datamodel/taxonmodel.py b/src/dendropy/datamodel/taxonmodel.py index e459a6c1b..1306cb8a3 100644 --- a/src/dendropy/datamodel/taxonmodel.py +++ b/src/dendropy/datamodel/taxonmodel.py @@ -1674,7 +1674,7 @@ def __init__(self, label=None): else: basemodel.DataObject.__init__(self, label=label) self._lower_cased_label = None - self.comments = [] + self.comments = [] def _get_label(self): return self._label diff --git a/tests/unittests/test_datamodel_taxon.py b/tests/unittests/test_datamodel_taxon.py index c9d97be05..46f15838f 100644 --- a/tests/unittests/test_datamodel_taxon.py +++ b/tests/unittests/test_datamodel_taxon.py @@ -130,6 +130,17 @@ def test_construct_from_another_with_complex_annotations(self): self.assertEqual(t2.annotations[1].value, "z") t1.label = "a" + def test_construct_from_another_preserves_comments(self): + # Regression test: Taxon.__init__() unconditionally reset + # 'self.comments = []' after the if/else branch that (for the + # Taxon-from-Taxon case) had just deep-copied 'comments' from the + # source Taxon, silently discarding it. + t1 = Taxon("a") + t1.comments.append("hello world") + for t2 in (Taxon(t1), copy.deepcopy(t1), t1.clone(2)): + self.assertIsNot(t1, t2) + self.assertEqual(t2.comments, ["hello world"]) + def test_simple_copy(self): t1 = Taxon("a") with self.assertRaises(TypeError): From 803f01073decc73eda93ba788a50dbb019525b20 Mon Sep 17 00:00:00 2001 From: Mike German Date: Sun, 12 Jul 2026 12:53:08 -0400 Subject: [PATCH 11/13] fix: Node.distance_from_root() no longer crashes on None parent edge length When a node's own edge length is None but it has a parent, the method fell back to float(self._parent_node.edge.length); if the parent's edge length was also None, this raised a TypeError instead of degrading to 0.0 the way the sibling no-parent/no-length branch already does. Signed-off-by: Mike German --- src/dendropy/datamodel/treemodel/_node.py | 2 ++ .../test_datamodel_tree_node_fundamentals.py | 16 ++++++++++++++++ 2 files changed, 18 insertions(+) diff --git a/src/dendropy/datamodel/treemodel/_node.py b/src/dendropy/datamodel/treemodel/_node.py index a1c8b8636..cec3b06c4 100644 --- a/src/dendropy/datamodel/treemodel/_node.py +++ b/src/dendropy/datamodel/treemodel/_node.py @@ -1374,6 +1374,8 @@ def distance_from_root(self): elif self._parent_node and self.edge.length == None: # what do we do here: parent node exists, but my # length does not? + if self._parent_node.edge.length == None: + return 0.0 return float(self._parent_node.edge.length) elif not self._parent_node and self.edge.length == None: # no parent node, and no edge length diff --git a/tests/unittests/test_datamodel_tree_node_fundamentals.py b/tests/unittests/test_datamodel_tree_node_fundamentals.py index 1ad4e8d74..630ee07cd 100644 --- a/tests/unittests/test_datamodel_tree_node_fundamentals.py +++ b/tests/unittests/test_datamodel_tree_node_fundamentals.py @@ -340,5 +340,21 @@ def test_edge_setting_does_not_corrupt_parent_child_bookkeeping(self): self.assertIn(child1, parent.child_nodes()) self.assertIs(child1.parent_node, parent) +class TestNodeDistanceFromRoot(unittest.TestCase): + + def test_none_edge_length_with_parent_also_none(self): + # Regression test: when a node's own edge length is None and it + # has a parent, distance_from_root() falls back to + # float(parent.edge.length); if the parent's edge length is also + # None, this raised a TypeError instead of degrading gracefully + # (as the sibling all-None-length branch a few lines down does, + # which returns 0.0). + parent = dendropy.Node(label="parent") + child = dendropy.Node(label="child") + parent.set_child_nodes([child]) + parent.edge.length = None + child.edge.length = None + self.assertEqual(child.distance_from_root(), 0.0) + if __name__ == "__main__": unittest.main() From a51cc19f1026f11d479c43b77cef92ccf620970f Mon Sep 17 00:00:00 2001 From: Mike German Date: Mon, 13 Jul 2026 10:08:01 -0400 Subject: [PATCH 12/13] =?UTF-8?q?test:=20address=20review=20=E2=80=94=20fi?= =?UTF-8?q?x=20docstrings,=20drop=20over-complex=20tests,=20verify=20colla?= =?UTF-8?q?pse=20behavior?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Applies @mmore500's review on #234: - taxonmodel/discrete: accept docstring suggestion edits - drop the SES-shuffle and is_bipartitions_updated deprecation tests (complicated tests for simple fixes, per request); the underlying source fixes stay - replace the collapse_negative_edges crash test with a behavioral test that asserts a negative edge (child older than parent) is actually collapsed by raising the parent to its oldest child's age --- src/dendropy/datamodel/taxonmodel.py | 2 +- src/dendropy/model/discrete.py | 2 +- .../test_phylogenetic_distance_matrix.py | 49 ---------- .../test_tree_summarization_and_consensus.py | 91 +++++++++++++------ 4 files changed, 67 insertions(+), 77 deletions(-) diff --git a/src/dendropy/datamodel/taxonmodel.py b/src/dendropy/datamodel/taxonmodel.py index 1306cb8a3..1fad2cc29 100644 --- a/src/dendropy/datamodel/taxonmodel.py +++ b/src/dendropy/datamodel/taxonmodel.py @@ -1660,7 +1660,7 @@ def __init__(self, label=None): string, then the ``label`` attribute of ``self`` is set to this value. If a |Taxon| object, then the ``label`` attribute of ``self`` is set to the same value as the ``label`` attribute the other - |Taxon| object and all annotations/metadata are copied. + |Taxon| object and all annotations/comments/metadata are copied. """ if isinstance(label, Taxon): other_taxon = label diff --git a/src/dendropy/model/discrete.py b/src/dendropy/model/discrete.py index b93d20cbb..1920c06b1 100644 --- a/src/dendropy/model/discrete.py +++ b/src/dendropy/model/discrete.py @@ -473,7 +473,7 @@ def simulate_discrete_chars( mutation_rate : float Mutation *modifier* rate (should be 1.0 if branch lengths on tree reflect true expected number of changes). - root_states`` : list + root_states : list Vector of root states (length must equal ``seq_len``). char_matrix : |DnaCharacterMatrix| If given, new sequences for taxa on ``tree_model`` leaf_nodes will be diff --git a/tests/unittests/test_phylogenetic_distance_matrix.py b/tests/unittests/test_phylogenetic_distance_matrix.py index f1b938ad8..969a88beb 100644 --- a/tests/unittests/test_phylogenetic_distance_matrix.py +++ b/tests/unittests/test_phylogenetic_distance_matrix.py @@ -339,55 +339,6 @@ def test_shuffle(self): self.assertEqual(pdc1, pdc2) self.assertNotEqual(pdc0, pdc1) -class PhylogeneticDistanceMatrixStandardizedEffectSizeShuffleTest(unittest.TestCase): - """ - Regression test for _calculate_standardized_effect_size(): for the - unweighted-edge-distance case, the null model randomization must - actually shuffle '_taxon_phylogenetic_path_steps' (the matrix that - unweighted statistics read from), not silently leave it untouched. - """ - - def setUp(self): - self.tree = dendropy.Tree.get_from_path( - src=pathmap.tree_source_path("community.tree.newick"), - schema="newick", - rooting="force-rooted") - self.pdm = dendropy.PhylogeneticDistanceMatrix.from_tree(self.tree) - taxa = list(self.pdm.taxon_iter()) - filter_fn = lambda taxon: taxon in set(taxa) - self.comparison_regime = list(self.pdm.distinct_taxon_pair_iter(filter_fn=filter_fn)) - - def test_unweighted_null_model_shuffles_path_steps(self): - import random - from dendropy.calculate.phylogeneticdistance import PhylogeneticDistanceMatrix - - observed_kwargs = [] - original_shuffle_taxa = PhylogeneticDistanceMatrix.shuffle_taxa - def recording_shuffle_taxa(self, *args, **kwargs): - observed_kwargs.append(dict(kwargs)) - return original_shuffle_taxa(self, *args, **kwargs) - PhylogeneticDistanceMatrix.shuffle_taxa = recording_shuffle_taxa - try: - self.pdm._calculate_standardized_effect_size( - statisticf_name="_calculate_mean_pairwise_distance", - comparison_regimes=[self.comparison_regime], - is_weighted_edge_distances=False, - is_normalize_by_tree_size=False, - num_randomization_replicates=3, - rng=random.Random(1), - ) - finally: - PhylogeneticDistanceMatrix.shuffle_taxa = original_shuffle_taxa - self.assertTrue(len(observed_kwargs) > 0) - for kwargs in observed_kwargs: - self.assertTrue( - kwargs["is_shuffle_phylogenetic_path_steps"], - "is_weighted_edge_distances=False must shuffle " - "phylogenetic path steps for the null model to be " - "meaningful (unweighted statistics read from " - "'_taxon_phylogenetic_path_steps', not " - "'_taxon_phylogenetic_distances')") - class TreePatristicDistTest(unittest.TestCase): def setUp(self): diff --git a/tests/unittests/test_tree_summarization_and_consensus.py b/tests/unittests/test_tree_summarization_and_consensus.py index 2509bb04f..86d6b0c09 100644 --- a/tests/unittests/test_tree_summarization_and_consensus.py +++ b/tests/unittests/test_tree_summarization_and_consensus.py @@ -26,7 +26,6 @@ import dendropy import random import itertools -import warnings from dendropy.calculate import treecompare from dendropy.calculate import statistics from dendropy.calculate import treesum @@ -253,31 +252,6 @@ def _get_target_tree(self): target_tree.encode_bipartitions() return target_tree - def test_is_bipartitions_updated_true_skips_reencoding(self): - # 'is_bipartitions_updated=True' used to trigger a *re*-encode via - # the deprecated 'encode_splits()' (inverted vs. every sibling - # summarization method in this module, which all skip re-encoding - # when the caller asserts bipartitions are already current). - target_tree = self._get_target_tree() - summarizer = treesum.TreeSummarizer() - with warnings.catch_warnings(record=True) as caught: - warnings.simplefilter("always") - summarizer.summarize_node_ages_on_tree( - tree=target_tree, - split_distribution=self.split_distribution, - set_edge_lengths=False, - collapse_negative_edges=False, - is_bipartitions_updated=True, - ) - deprecation_warnings = [ - w for w in caught - if "encode_splits" in str(w.message)] - self.assertEqual( - len(deprecation_warnings), 0, - "summarize_node_ages_on_tree(is_bipartitions_updated=True) " - "should not re-encode bipartitions via the deprecated " - "encode_splits()") - def test_collapse_negative_edges_does_not_crash(self): # 'collapse_negative_edges=True' used to compare a not-yet-visited # child's 'age' (still None, since ages are assigned in preorder) @@ -295,6 +269,71 @@ def test_collapse_negative_edges_does_not_crash(self): if nd.parent_node is not None: self.assertGreaterEqual(nd.parent_node.age, nd.age) + def test_collapse_negative_edges_raises_parent_to_oldest_child(self): + # 'collapse_negative_edges=True' must actually *collapse* negative + # edges, not merely avoid crashing: when the summarized age of an + # internal node comes out older than its parent's summarized age + # (a negative edge), the parent's age is raised to its oldest + # child's age. Here the (B,C) split is summarized deep while the + # root split is summarized shallow, so without collapsing the root + # ends up younger than its (B,C) child. + support_newicks = [ + "((A:0.1,B:0.1):0.1,C:0.2);", # topology ((A,B),C); root age 0.2, no (B,C) split + "((A:0.1,B:0.1):0.1,C:0.2);", # again, to keep the root/full-split age shallow + "(A:1.0,(B:0.9,C:0.9):0.1);", # topology (A,(B,C)); (B,C) split age 0.9 + ] + split_distribution = dendropy.SplitDistribution( + taxon_namespace=self.taxon_namespace, + ignore_node_ages=False) + for newick in support_newicks: + tree = dendropy.Tree.get( + data=newick, + schema="newick", + taxon_namespace=self.taxon_namespace, + rooting="force-rooted") + tree.encode_bipartitions() + tree.calc_node_ages() + split_distribution.count_splits_on_tree( + tree, is_bipartitions_updated=True) + + def summarize(collapse_negative_edges): + target_tree = dendropy.Tree.get( + data="(A:1,(B:1,C:1):1);", + schema="newick", + taxon_namespace=self.taxon_namespace, + rooting="force-rooted") + target_tree.encode_bipartitions() + treesum.TreeSummarizer().summarize_node_ages_on_tree( + tree=target_tree, + split_distribution=split_distribution, + set_edge_lengths=False, + collapse_negative_edges=collapse_negative_edges, + is_bipartitions_updated=True, + ) + ages = {} + for nd in target_tree.preorder_node_iter(): + key = frozenset( + leaf.taxon.label for leaf in nd.leaf_iter()) + ages[key] = nd.age + return ages + + root_key = frozenset(["A", "B", "C"]) + internal_key = frozenset(["B", "C"]) + + # Sanity check on the constructed fixture: without collapsing, the + # (B,C) child is summarized older than the root, i.e. a negative edge. + uncollapsed = summarize(collapse_negative_edges=False) + self.assertGreater( + uncollapsed[internal_key], uncollapsed[root_key], + "fixture should produce a negative edge (child older than " + "parent) when negative edges are not collapsed") + + # With collapsing, the parent's age is raised to its oldest child's + # age, so no node is older than its parent. + collapsed = summarize(collapse_negative_edges=True) + self.assertEqual(collapsed[root_key], uncollapsed[internal_key]) + self.assertGreaterEqual(collapsed[root_key], collapsed[internal_key]) + class TestTopologyCounter(dendropytest.ExtendedTestCase): def get_regime(self, From ae2c0f49476c9ae9eac35df319f259b7d0a7f298 Mon Sep 17 00:00:00 2001 From: Mike German Date: Mon, 13 Jul 2026 10:08:07 -0400 Subject: [PATCH 13/13] revert: split Node.edge parent/children fix into its own branch Per @mmore500's review on #234, the change that stopped Node._set_edge() from removing the node from its parent's _child_nodes list needs its own discussion (whether _parent_node should also be reset). Moved to branch fix/node-parent-reindex; removed here along with its regression test. The unrelated distance_from_root() None-parent fix stays in this sweep. --- src/dendropy/datamodel/treemodel/_node.py | 5 +++++ .../test_datamodel_tree_node_fundamentals.py | 18 ------------------ 2 files changed, 5 insertions(+), 18 deletions(-) diff --git a/src/dendropy/datamodel/treemodel/_node.py b/src/dendropy/datamodel/treemodel/_node.py index cec3b06c4..427f73df9 100644 --- a/src/dendropy/datamodel/treemodel/_node.py +++ b/src/dendropy/datamodel/treemodel/_node.py @@ -979,6 +979,11 @@ def _set_edge(self, new_edge): # raise ValueError("A Node cannot have 'None' for an edge") if new_edge is self._edge: return + if self._parent_node is not None: + try: + self._parent_node._child_nodes.remove(self) + except ValueError: + pass ## Minimal management self._edge = new_edge diff --git a/tests/unittests/test_datamodel_tree_node_fundamentals.py b/tests/unittests/test_datamodel_tree_node_fundamentals.py index 630ee07cd..da1a0dd8a 100644 --- a/tests/unittests/test_datamodel_tree_node_fundamentals.py +++ b/tests/unittests/test_datamodel_tree_node_fundamentals.py @@ -322,24 +322,6 @@ def test_edge_head_node_setting(self): self.assertIs(node.edge, edge2) self.assertIs(node.edge.head_node, node) - def test_edge_setting_does_not_corrupt_parent_child_bookkeeping(self): - # Reassigning a node's ``edge`` is unrelated to the node's position - # in the tree, and should not affect the parent's list of children - # or the node's own parent reference. - parent = dendropy.Node(label="parent") - child1 = dendropy.Node(label="child1") - child2 = dendropy.Node(label="child2") - parent.set_child_nodes([child1, child2]) - self.assertEqual(len(parent.child_nodes()), 2) - self.assertIn(child1, parent.child_nodes()) - self.assertIs(child1.parent_node, parent) - - child1.edge = dendropy.Edge(length=5.0) - - self.assertEqual(len(parent.child_nodes()), 2) - self.assertIn(child1, parent.child_nodes()) - self.assertIs(child1.parent_node, parent) - class TestNodeDistanceFromRoot(unittest.TestCase): def test_none_edge_length_with_parent_also_none(self):