Phylo export methods cleanup

- Remove TreeSequence.to_nexus and add TreeSequence.as_nexus
- Add Tree.as_newick and refactor
- Update C newick generation to cover new defaults
- Refactor newick testing code paths

Closes #1803
Closes #1815
Closes #1671

Author: jeromekelleher

c/tests/test_convert.c

@@ -45,20 +45,31 @@ test_single_tree_newick(void)
     ret = tsk_tree_first(&t);
     CU_ASSERT_EQUAL_FATAL(ret, 1)
 
-    ret = tsk_convert_newick(&t, 0, 0, 0, buffer_size, newick);
+    ret = tsk_convert_newick(&t, 0, 0, TSK_NEWICK_LEGACY_MS_LABELS, buffer_size, newick);
     CU_ASSERT_EQUAL_FATAL(ret, 0);
     /* Seems odd, but this is what a single node newick tree looks like.
      * Newick parsers seems to accept it in any case */
     CU_ASSERT_STRING_EQUAL(newick, "1;");
 
-    ret = tsk_convert_newick(&t, 4, 0, 0, buffer_size, newick);
+    ret = tsk_convert_newick(&t, 0, 0, 0, buffer_size, newick);
+    CU_ASSERT_EQUAL_FATAL(ret, 0);
+    CU_ASSERT_STRING_EQUAL(newick, "n0;");
+
+    ret = tsk_convert_newick(&t, 4, 0, TSK_NEWICK_LEGACY_MS_LABELS, buffer_size, newick);
     CU_ASSERT_EQUAL_FATAL(ret, 0);
     CU_ASSERT_STRING_EQUAL(newick, "(1:1,2:1);");
+    ret = tsk_convert_newick(&t, 4, 0, 0, buffer_size, newick);
+    CU_ASSERT_EQUAL_FATAL(ret, 0);
+    CU_ASSERT_STRING_EQUAL(newick, "(n0:1,n1:1);");
 
-    ret = tsk_convert_newick(&t, 6, 0, 0, buffer_size, newick);
+    ret = tsk_convert_newick(&t, 6, 0, TSK_NEWICK_LEGACY_MS_LABELS, buffer_size, newick);
     CU_ASSERT_EQUAL_FATAL(ret, 0);
     CU_ASSERT_STRING_EQUAL(newick, "((1:1,2:1):2,(3:2,4:2):1);");
 
+    ret = tsk_convert_newick(&t, 6, 0, 0, buffer_size, newick);
+    CU_ASSERT_EQUAL_FATAL(ret, 0);
+    CU_ASSERT_STRING_EQUAL(newick, "((n0:1,n1:1):2,(n2:2,n3:2):1);");
+
     tsk_tree_free(&t);
     tsk_treeseq_free(&ts);
 }
@@ -95,7 +106,8 @@ test_single_tree_newick_errors(void)
         ret = tsk_convert_newick(&t, 6, 0, 0, j, newick);
         CU_ASSERT_EQUAL_FATAL(ret, TSK_ERR_BUFFER_OVERFLOW);
     }
-    ret = tsk_convert_newick(&t, 6, 0, 0, len, newick);
+    ret = tsk_convert_newick(&t, 6, 0, TSK_NEWICK_LEGACY_MS_LABELS, len, newick);
+
     CU_ASSERT_EQUAL_FATAL(ret, 0);
     CU_ASSERT_STRING_EQUAL(newick, "((1:1,2:1):2,(3:2,4:2):1);");
 

c/tskit/convert.c

@@ -57,12 +57,15 @@ tsk_newick_converter_run(
     const tsk_tree_t *tree = self->tree;
     tsk_id_t *stack = self->traversal_stack;
     const double *time = self->tree->tree_sequence->tables->nodes.time;
+    const tsk_flags_t *flags = self->tree->tree_sequence->tables->nodes.flags;
     int stack_top = 0;
     int label;
     size_t s = 0;
     int r;
     tsk_id_t u, v, w, root_parent;
     double branch_length;
+    bool ms_labels = self->options & TSK_NEWICK_LEGACY_MS_LABELS;
+    const char *label_format = ms_labels ? "%d" : "n%d";
 
     if (root < 0 || root >= (tsk_id_t) self->tree->num_nodes) {
         ret = TSK_ERR_NODE_OUT_OF_BOUNDS;
@@ -91,15 +94,20 @@ tsk_newick_converter_run(
         } else {
             u = tree->parent[v];
             stack_top--;
-            if (tree->left_child[v] == TSK_NULL) {
+            label = -1;
+            if (ms_labels) {
+                if (tree->left_child[v] == TSK_NULL) {
+                    label = (int) v + 1;
+                }
+            } else if (flags[v] & TSK_NODE_IS_SAMPLE) {
+                label = (int) v;
+            }
+            if (label != -1) {
                 if (s >= buffer_size) {
                     ret = TSK_ERR_BUFFER_OVERFLOW;
                     goto out;
                 }
-                /* We do this for ms-compatability. This should be a configurable option
-                 * via the flags attribute */
-                label = (int) v + 1;
-                r = snprintf(buffer + s, buffer_size - s, "%d", label);
+                r = snprintf(buffer + s, buffer_size - s, label_format, label);
                 if (r < 0) {
                     ret = TSK_ERR_IO;
                     goto out;

c/tskit/convert.h

@@ -32,6 +32,8 @@ extern "C" {
 
 #include <tskit/trees.h>
 
+#define TSK_NEWICK_LEGACY_MS_LABELS (1 << 0)
+
 int tsk_convert_newick(const tsk_tree_t *tree, tsk_id_t root, unsigned int precision,
     tsk_flags_t options, size_t buffer_size, char *buffer);
 

python/CHANGELOG.rst

@@ -1,5 +1,5 @@
 --------------------
-[0.3.8] - 2021-0X-XX
+[0.4.0] - 2021-1X-XX
 --------------------
 
 **Breaking changes**
@@ -37,10 +37,12 @@
 - For ``TreeSequence.samples`` all arguments after ``population`` are now keyword only
   (:user:`benjeffery`, :issue:`1715`, :pr:`1831`).
 
-- Fix bugs in the format produced by ``TreeSequence.to_nexus`` to make
-  it standards-compliant. (:user:`jeetsukumaran`, :user:`jeromekelleher`,
-  :issue:`1785`, :pr:`1835`, :pr:`1836`)
-  [FIXME MORE UPDATES HERE AS WE CHANGE THE LABELS ETC]
+- Remove the method ``TreeSequence.to_nexus`` and replace with ``TreeSequence.as_nexus``.
+  As the old method was not generating standards-compliant output, it seems unlikely
+  that it was used by anyone. Calls to ``to_nexus`` will result in a
+  NotImplementedError, informing users of the change. See below for details on
+  ``as_nexus``.
+
 
 **Features**
 
@@ -84,6 +86,17 @@
 
 - `dump_tables` omitted individual parents. (:user:`benjeffery`, :issue:`1828`, :pr:`1884`)
 
+- Add the ``Tree.as_newick`` method and deprecate ``Tree.newick``. The
+  ``as_newick`` method by default labels samples with the pattern ``"n{node_id}"``
+  which is much more useful that the behaviour of ``Tree.newick`` (which mimics
+  ``ms`` output). (:user:`jeromekelleher`, :issue:`1671`, :pr:`1838`.)
+
+- Add the ``as_nexus`` and ``write_nexus`` methods to the TreeSequence class,
+  replacing the broken ``to_nexus`` method (see above). This uses the same
+  sample labelling pattern as ``as_newick``.
+  (:user:`jeetsukumaran`, :user:`jeromekelleher`, :issue:`1785`, :pr:`1835`,
+  :pr:`1836`, :pr:`1838`)
+
 --------------------
 [0.3.7] - 2021-07-08
 --------------------

python/_tskitmodule.c

@@ -9954,24 +9954,27 @@ static PyObject *
 Tree_get_newick(Tree *self, PyObject *args, PyObject *kwds)
 {
     PyObject *ret = NULL;
-    static char *kwlist[] = { "root", "precision", "buffer_size", NULL };
+    static char *kwlist[]
+        = { "root", "precision", "buffer_size", "legacy_ms_labels", NULL };
     int precision = 14;
     /* We have a default bufsize for convenience, but the high-level code
      * should set this by computing an upper bound. */
     Py_ssize_t buffer_size = 1024;
     int root, err;
     char *buffer = NULL;
+    int legacy_ms_labels = false;
+    tsk_flags_t options = 0;
 
     if (Tree_check_state(self) != 0) {
         goto out;
     }
-    if (!PyArg_ParseTupleAndKeywords(
-            args, kwds, "i|in", kwlist, &root, &precision, &buffer_size)) {
+    if (!PyArg_ParseTupleAndKeywords(args, kwds, "i|ini", kwlist, &root, &precision,
+            &buffer_size, &legacy_ms_labels)) {
         goto out;
     }
-    if (precision < 0 || precision > 16) {
+    if (precision < 0 || precision > 17) {
         PyErr_SetString(
-            PyExc_ValueError, "Precision must be between 0 and 16, inclusive");
+            PyExc_ValueError, "Precision must be between 0 and 17, inclusive");
         goto out;
     }
     if (buffer_size <= 0) {
@@ -9982,13 +9985,16 @@ Tree_get_newick(Tree *self, PyObject *args, PyObject *kwds)
     if (buffer == NULL) {
         PyErr_NoMemory();
     }
+    if (legacy_ms_labels) {
+        options |= TSK_NEWICK_LEGACY_MS_LABELS;
+    }
     err = tsk_convert_newick(
-        self->tree, (tsk_id_t) root, precision, 0, (size_t) buffer_size, buffer);
+        self->tree, (tsk_id_t) root, precision, options, (size_t) buffer_size, buffer);
     if (err != 0) {
         handle_library_error(err);
         goto out;
     }
-    ret = PyBytes_FromString(buffer);
+    ret = PyUnicode_FromString(buffer);
 out:
     if (buffer != NULL) {
         PyMem_Free(buffer);

python/tests/__init__.py

@@ -118,26 +118,6 @@ def __eq__(self, other):
     def __ne__(self, other):
         return not self.__eq__(other)
 
-    def newick(self, root=None, precision=16, node_labels=None):
-        if node_labels is None:
-            node_labels = {u: str(u + 1) for u in self.tree.leaves()}
-        if root is None:
-            root = self.left_root
-        return self._build_newick(root, precision, node_labels) + ";"
-
-    def _build_newick(self, node, precision, node_labels):
-        label = node_labels.get(node, "")
-        if self.left_child[node] == tskit.NULL:
-            s = label
-        else:
-            s = "("
-            for child in self.children(node):
-                branch_length = self.tree.branch_length(child)
-                subtree = self._build_newick(child, precision, node_labels)
-                s += subtree + ":{0:.{1}f},".format(branch_length, precision)
-            s = s[:-1] + label + ")"
-        return s
-
 
 class PythonTreeSequence:
     """

python/tests/test_highlevel.py

@@ -1951,6 +1951,8 @@ def test_removed_methods(self):
             ts.diffs()
         with pytest.raises(NotImplementedError):
             ts.newick_trees()
+        with pytest.raises(NotImplementedError):
+            ts.to_nexus()
 
     def test_dump_pathlib(self, ts_fixture, tmp_path):
         path = tmp_path / "tmp.trees"
@@ -2955,121 +2957,6 @@ def test_root_properties(self):
                     assert tree.root == tskit.NULL
         assert len(tested) == 3
 
-    def verify_newick(self, tree):
-        """
-        Verifies that we output the newick tree as expected.
-        """
-        # TODO to make this work we may need to clamp the precision of node
-        # times because Python and C float printing algorithms work slightly
-        # differently. Seems to work OK now, so leaving alone.
-        if tree.has_single_root:
-            py_tree = tests.PythonTree.from_tree(tree)
-            newick1 = tree.newick(precision=16)
-            newick2 = py_tree.newick()
-            assert newick1 == newick2
-
-            # Make sure we get the same results for a leaf root.
-            newick1 = tree.newick(root=0, precision=16)
-            newick2 = py_tree.newick(root=0)
-            assert newick1 == newick2
-
-            # When we specify the node_labels we should get precisely the
-            # same result as we are using Python code now.
-            for precision in [0, 3, 19]:
-                newick1 = tree.newick(precision=precision, node_labels={})
-                newick2 = py_tree.newick(precision=precision, node_labels={})
-                assert newick1 == newick2
-        else:
-            with pytest.raises(ValueError):
-                tree.newick()
-            for root in tree.roots:
-                py_tree = tests.PythonTree.from_tree(tree)
-                newick1 = tree.newick(precision=16, root=root)
-                newick2 = py_tree.newick(root=root)
-                assert newick1 == newick2
-
-    def test_newick(self):
-        for ts in get_example_tree_sequences():
-            for tree in ts.trees():
-                self.verify_newick(tree)
-
-    def test_newick_large_times(self):
-        for n in [2, 10, 20, 100]:
-            ts = msprime.simulate(n, Ne=100e6, random_seed=1)
-            tree = ts.first()
-            for precision in [0, 1, 16]:
-                newick_py = tree.newick(
-                    node_labels={u: str(u + 1) for u in ts.samples()},
-                    precision=precision,
-                )
-                newick_c = tree.newick(precision=precision)
-                assert newick_c == newick_py
-
-    def test_bifurcating_newick(self):
-        for n_tips in range(2, 6):
-            ts = msprime.simulate(n_tips, random_seed=1)  # msprime trees are binary
-            for tree in ts.trees():
-                base_newick = tree.newick(include_branch_lengths=False).strip(";")
-                for i in range(n_tips):
-                    # Each tip number (i+1) mentioned once
-                    assert base_newick.count(str(i + 1)) == 1
-                # Binary newick trees have 3 chars per extra tip: "(,)"
-                assert len(base_newick) == n_tips + 3 * (n_tips - 1)
-
-    def test_newick_topology_equiv(self):
-        replace_numeric = {ord(x): None for x in "1234567890:."}
-        for ts in get_example_tree_sequences():
-            for tree in ts.trees():
-                if not tree.has_single_root:
-                    with pytest.raises(ValueError) as plain_newick_err:
-                        tree.newick(node_labels={}, include_branch_lengths=False)
-                    with pytest.raises(ValueError) as newick1_err:
-                        tree.newick()
-                    with pytest.raises(ValueError) as newick2_err:
-                        tree.newick(node_labels={})
-                    assert str(newick1_err) == str(newick2_err)
-                    assert str(newick1_err) == str(plain_newick_err)
-                else:
-                    plain_newick = tree.newick(
-                        node_labels={}, include_branch_lengths=False
-                    )
-                    newick1 = tree.newick().translate(replace_numeric)
-                    newick2 = tree.newick(node_labels={}).translate(replace_numeric)
-                    assert newick1 == newick2
-                    assert newick2 == plain_newick
-
-    def test_newick_buffer_too_small_bug(self):
-        nodes = io.StringIO(
-            """\
-        id  is_sample   population individual time
-        0       1       0       -1      0.00000000000000
-        1       1       0       -1      0.00000000000000
-        2       1       0       -1      0.00000000000000
-        3       1       0       -1      0.00000000000000
-        4       0       0       -1      0.21204940078588
-        5       0       0       -1      0.38445004304611
-        6       0       0       -1      0.83130278081275
-        """
-        )
-        edges = io.StringIO(
-            """\
-        id      left            right           parent  child
-        0       0.00000000      1.00000000      4       0
-        1       0.00000000      1.00000000      4       2
-        2       0.00000000      1.00000000      5       1
-        3       0.00000000      1.00000000      5       3
-        4       0.00000000      1.00000000      6       4
-        5       0.00000000      1.00000000      6       5
-        """
-        )
-        ts = tskit.load_text(nodes, edges, sequence_length=1, strict=False)
-        tree = ts.first()
-        for precision in range(17):
-            newick_c = tree.newick(precision=precision)
-            node_labels = {u: str(u + 1) for u in ts.samples()}
-            newick_py = tree.newick(precision=precision, node_labels=node_labels)
-            assert newick_c == newick_py
-
     def test_as_dict_of_dicts(self):
         for ts in get_example_tree_sequences():
             tree = next(ts.trees())