Fix calculating height of clades when building UPGMA tree version 2 (b…

…iopython#3951)

* Fixed a bug in calculating the height of clades when building a UPGMA tree

* added my name to contrib.rst alphabetically

* minor corrections

* formatting changes

* Add tests

---------

Co-authored-by: She Zhang <shz66@pitt.edu>
Co-authored-by: Fabian Egli <fabianegli@users.noreply.github.com>
Co-authored-by: Eric Talevich <etal@users.noreply.github.com>

Bio/Phylo/TreeConstruction.py

@@ -745,15 +745,8 @@ def upgma(self, distance_matrix):
             inner_clade.clades.append(clade1)
             inner_clade.clades.append(clade2)
             # assign branch length
-            if clade1.is_terminal():
-                clade1.branch_length = min_dist / 2
-            else:
-                clade1.branch_length = min_dist / 2 - self._height_of(clade1)
-
-            if clade2.is_terminal():
-                clade2.branch_length = min_dist / 2
-            else:
-                clade2.branch_length = min_dist / 2 - self._height_of(clade2)
+            clade1.branch_length = min_dist * 1.0 / 2 - self._height_of(clade1)
+            clade2.branch_length = min_dist * 1.0 / 2 - self._height_of(clade2)
 
             # update node list
             clades[min_j] = inner_clade
@@ -876,11 +869,11 @@ def nj(self, distance_matrix):
 
     def _height_of(self, clade):
         """Calculate clade height -- the longest path to any terminal (PRIVATE)."""
-        height = 0
         if clade.is_terminal():
-            height = clade.branch_length
+            height = 0
         else:
-            height = height + max(self._height_of(c) for c in clade.clades)
+            height = max(self._height_of(c) + c.branch_length for c in clade.clades)
+
         return height
 
 

CONTRIB.rst

@@ -300,6 +300,7 @@ please open an issue on GitHub or mention it on the mailing list.
 - Sergei Lebedev <https://github.com/superbobry>
 - Sergio Valqui <https://github.com/svalqui>
 - Seth Sims <seth.sims at gmail>
+- She Zhang <https://github.com/shz66>
 - Shoichiro Kawauchi <https://github.com/lacrosse91>
 - Shuichiro MAKIGAKI <https://github.com/shuichiro-makigaki>
 - Shyam Saladi <https://github.com/smsaladi>

Tests/test_TreeConstruction.py

@@ -9,6 +9,7 @@
 import tempfile
 import unittest
 from io import StringIO
+from itertools import combinations
 
 from Bio import Align
 from Bio import AlignIO
@@ -232,7 +233,13 @@ def test_upgma(self):
         # Phylo.write(tree, tree_file, 'newick')
         ref_tree = Phylo.read("./TreeConstruction/upgma.tre", "newick")
         self.assertTrue(Consensus._equal_topology(tree, ref_tree))
-        # ref_tree.close()
+        # check for equal distance of all terminal nodes from the root
+        ref_tree.root_at_midpoint()
+        for len1, len2 in combinations(
+            [depth for node, depth in ref_tree.depths().items() if node.is_terminal()],
+            2,
+        ):
+            self.assertAlmostEqual(len1, len2)
 
     def test_nj_msa(self):
         tree = self.constructor.nj(self.dm_msa)