Repository: cogent3/cogent3 (python)

Issue:
BUG: newick trees with the same support values are incorrectly assigned suffixes
**Describe the bug**
When a supported newick tree provided, the parser will incorrectly add suffix to nodes with the same support value.

**To Reproduce**
```
>>> import cogent3

>>> tree = cogent3.make_tree("((a,b)100, (c,d)100);", name_nodes=True)
>>> print(tree.to_rich_dict())

```
it gives:

"{'newick': '((a,b)node1,(c,d)node2)', 'edge_attributes': {'a': {'length': None}, 'b': {'length': None}, 
'node1': {'length': None, 'support': **_100.0_**}, 'c': {'length': None}, 'd': {'length': None}, 
'node2': {'length': None, 'support': **_100.2_**}, 
'root': {'length': None}}, 'type': 'cogent3.core.tree.PhyloNode', 'version': '2024.7.19a9'}"


Pull request:
DEV: process token in tree node classes to handle incorrect identification of support or name when creating a node
In this PR:
- Added the static method `parse_token` to both TreeNode and PhyloNode.
- Applied `parse_token` in `TreeBuilder.create_edge()` to distinguish between name and support before creating a node.
- Modified `test_phylonode_support` to cover cases where the same support value occurs.

- Deprecated `name_node` parameter in `make_tree` and its tests, as nodes with only support are now always assigned names.

closes #2116

## Summary by Sourcery

Add a static method `parse_token` to TreeNode and PhyloNode classes to improve token parsing and fix node creation issues. Apply this method in `TreeBuilder.create_edge()` to correctly distinguish between name and support. Update tests to cover new scenarios and deprecate the `name_node` parameter in `make_tree`.

New Features:
- Introduce a static method `parse_token` in both TreeNode and PhyloNode classes to handle token parsing.

Bug Fixes:
- Fix incorrect identification of support or name when creating a node by applying `parse_token` in `TreeBuilder.create_edge()`.

Enhancements:
- Modify `test_phylonode_support` to cover cases where the same support value occurs.

Chores:
- Deprecate the `name_node` parameter in `make_tree` as nodes with only support are now always assigned names.

Patch:
diff --git a/src/cogent3/__init__.py b/src/cogent3/__init__.py
index d55f3ec7e..f8189d5ce 100644
--- a/src/cogent3/__init__.py
+++ b/src/cogent3/__init__.py
@@ -30,9 +30,9 @@ from cogent3.core.moltype import (
     get_moltype,  # noqa
 )
 from cogent3.core.tree import PhyloNode, TreeBuilder, TreeError, TreeNode
-from cogent3.evolve.fast_distance import (  # noqa
-    available_distances,
-    get_distance_calculator,
+from cogent3.evolve.fast_distance import (
+    available_distances,  # noqa
+    get_distance_calculator,  # noqa
 )
 from cogent3.evolve.models import available_models, get_model  # noqa
 from cogent3.parse.cogent3_json import load_from_json
@@ -40,6 +40,7 @@ from cogent3.parse.newick import parse_string as newick_parse_string
 from cogent3.parse.sequence import get_parser, is_genbank
 from cogent3.parse.table import load_delimited
 from cogent3.parse.tree_xml import parse_string as tree_xml_parse_string
+from cogent3.util import warning as c3warn
 from cogent3.util.io import get_format_suffixes, open_
 from cogent3.util.progress_display import display_wrap
 from cogent3.util.table import Table as _Table
@@ -108,7 +109,10 @@ def make_seq(
     else:
         moltype = get_moltype(moltype)
     seq = moltype.make_seq(
-        seq=seq, name=name, annotation_offset=annotation_offset, **kw
+        seq=seq,
+        name=name,
+        annotation_offset=annotation_offset,
+        **kw,
     )
     if annotation_db:
         seq.annotation_db = annotation_db
@@ -116,7 +120,13 @@ def make_seq(
 
 
 def _make_seq_container(
-    klass, data, moltype=None, label_to_name=None, info=None, source=None, **kw
+    klass,
+    data,
+    moltype=None,
+    label_to_name=None,
+    info=None,
+    source=None,
+    **kw,
 ):
     """utility function for creating the different sequence collection/alignment instances"""
     if moltype is not None:
@@ -131,12 +141,22 @@ def _make_seq_container(
     info["source"] = str(source)
 
     return klass(
-        data=data, moltype=moltype, label_to_name=label_to_name, info=info, **kw
+        data=data,
+        moltype=moltype,
+        label_to_name=label_to_name,
+        info=info,
+        **kw,
     )
 
 
 def make_unaligned_seqs(
-    data, moltype=None, label_to_name=None, info=None, source=None, new_type=False, **kw
+    data,
+    moltype=None,
+    label_to_name=None,
+    info=None,
+    source=None,
+    new_type=False,
+    **kw,
 ):
     """Initialize an unaligned collection of sequences.
 
@@ -299,7 +319,9 @@ T = Optional[_anno_db.SupportsFeatures]
 
 
 def _load_genbank_seq(
-    filename: os.PathLike, parser_kw: dict, just_seq: bool = False
+    filename: os.PathLike,
+    parser_kw: dict,
+    just_seq: bool = False,
 ) -> tuple[str, str, T]:
     """utility function for loading sequences"""
     from cogent3.parse.genbank import iter_genbank_records
@@ -313,7 +335,8 @@ def _load_genbank_seq(
         None
         if just_seq
         else _anno_db.GenbankAnnotationDb(
-            data=features.pop("features", None), seqid=name
+            data=features.pop("features", None),
+            seqid=name,
         )
     )
     return name, seq, db
@@ -378,7 +401,9 @@ def load_seq(
 
     if is_genbank(format or file_format):
         name, seq, db = _load_genbank_seq(
-            filename, parser_kw or {}, just_seq=annotation_path is not None
+            filename,
+            parser_kw or {},
+            just_seq=annotation_path is not None,
         )
     else:
         db = None
@@ -675,7 +700,7 @@ def load_table(
     """
     if not any(isinstance(filename, t) for t in (str, pathlib.PurePath)):
         raise TypeError(
-            "filename must be string or Path, perhaps you want make_table()"
+            "filename must be string or Path, perhaps you want make_table()",
         )
 
     sep = sep or kwargs.pop("delimiter", None)
@@ -703,7 +728,10 @@ def load_table(
             sep = sep or "\t"
 
         header, rows, loaded_title, legend = load_delimited(
-            filename, sep=sep, limit=limit, **kwargs
+            filename,
+            sep=sep,
+            limit=limit,
+            **kwargs,
         )
         if skip_inconsistent:
             num_fields = len(header)
@@ -735,12 +763,16 @@ def load_table(
     )
 
 
+@c3warn.deprecated_args(
+    version="2024.12",
+    reason="argument has no effect",
+    discontinued="name_nodes",
+)
 def make_tree(
     treestring=None,
     tip_names=None,
     format=None,
     underscore_unmunge=False,
-    name_nodes=False,
 ):
     """Initialises a tree.
 
@@ -756,8 +788,6 @@ def make_tree(
     underscore_unmunge : bool
         replace underscores with spaces in all names read, i.e. "sp_name"
         becomes "sp name"
-    name_nodes: bool
-        whether to name unnamed nodes
 
     Notes
     -----
@@ -787,15 +817,13 @@ def make_tree(
     if not tree.name_loaded:
         tree.name = "root"
 
-    # ensure all nodes have names if name_nodes is True
-    if name_nodes:
-        tree.name_unnamed_nodes()
-
     return tree
 
 
 def load_tree(
-    filename: Union[str, pathlib.Path], format=None, underscore_unmunge=False
+    filename: Union[str, pathlib.Path],
+    format=None,
+    underscore_unmunge=False,
 ):
     """Constructor for tree.
 
diff --git a/src/cogent3/core/tree.py b/src/cogent3/core/tree.py
index c3c88fb90..bfd2742e5 100644
--- a/src/cogent3/core/tree.py
+++ b/src/cogent3/core/tree.py
@@ -1695,6 +1695,11 @@ class TreeNode:
         """return dict of {<child name>: <parent name>, ...}"""
         return {e.name: e.parent.name for e in self.postorder(include_self=False)}
 
+    @staticmethod
+    def parse_token(token: str) -> tuple[str | None, dict]:
+        # TreeNode token contains no attributes
+        return token, {}
+
 
 class PhyloNode(TreeNode):
     def __init__(self, *args, **kwargs):
@@ -1704,13 +1709,6 @@ class PhyloNode(TreeNode):
         kwargs["params"] = params
         super().__init__(*args, **kwargs)
 
-        if self.children:
-            # self is an internal tree node
-            name, support = split_name_and_support(self.name)
-            self.name = name
-            if support is not None:
-                self.params["support"] = support
-
     @property
     def length(self) -> Union[float, None]:
         return self.params.get("length", None)
@@ -2192,6 +2190,11 @@ class PhyloNode(TreeNode):
                     tip_b[0] += child_b.length or 0.0
                 n.MaxDistTips = [tip_a, tip_b]
 
+    @staticmethod
+    def parse_token(token: str) -> tuple[str | None, dict]:
+        name, attrs = split_name_and_support(token)
+        return (name, {"support": attrs}) if attrs else (name, {})
+
 
 def split_name_and_support(name_field: str | None) -> tuple[str | None, float | None]:
     """Handle cases in the Newick format where an internal node name field
@@ -2270,6 +2273,11 @@ class TreeBuilder(object):
         """Callback for newick parser"""
         if children is None:
             children = []
+        # split name and support for internal nodes
+        elif children != []:
+            name, pars = self.TreeNodeClass.parse_token(name)
+            params |= pars
+
         node = self.TreeNodeClass(
             children=list(children),
             name=self._unique_name(name),


Test patch:
diff --git a/tests/test_core/test_tree.py b/tests/test_core/test_tree.py
index d70e61cf6..55db57503 100644
--- a/tests/test_core/test_tree.py
+++ b/tests/test_core/test_tree.py
@@ -2369,35 +2369,29 @@ def test_split_name_and_support_invalid_support(invalid):
 def test_phylonode_support():
     tip_names = [str(i) for i in range(1, 13)]
     tree = make_tree(
-        treestring="(1,(((2,3),4)/53,(5,((6,(7,(8,9))def/25),(10,11)abc))),12);"
+        treestring="(1,(((2,3)53,4)/53,(5,((6,(7,(8,9))def/25),(10,11)abc))),12);"
     )
     assert tree.get_tip_names() == tip_names
+
+    node_names = set(tree.get_node_names())
+    # check that no node name is an empty string or None
+    assert all(node_names)
+
     # parent of 4 is node with only support value
     just_support = tree.get_node_matching_name("4").parent
     assert just_support.params["support"] == 53.0
+
+    # parent of 2 has the same support as parent of 4
+    same_support = tree.get_node_matching_name("2").parent
+    assert same_support.params["support"] == just_support.params["support"]
+
     # parent of 10 has a node name only
     just_name = tree.get_node_matching_name("10").parent
     assert just_name.name == "abc"
     assert "support" not in just_name.params
+
     # the node with name "def/25" correctly resoloved into node
     # name "def" and support 25.0
     name_and_support = tree.get_node_matching_name("def")
     assert name_and_support.name == "def"  # bit redundant given selection process
     assert name_and_support.params["support"] == 25.0
-
-
-def test_phylonode_support_name_nodes_false():
-    # test that internal node names are None with name_nodes=False
-    tree = make_tree("((1,2)5,(3,4)6);", name_nodes=False)
-    internal_nodes = [node.name for node in tree.iter_nontips()]
-    assert all(node is None for node in internal_nodes)
-
-
-def test_phylonode_support_name_nodes_true():
-    # test that all nodes have unique names with name_nodes=True
-    tree = make_tree("((1,2)5,(3,4)6);", name_nodes=True)
-    node_names = set(tree.get_node_names())
-    # check that no node name is an empty string or None
-    assert all(node_names)
-    # check that the total number of unique node names is 7
-    assert len(node_names) == 7