Address review comments

Author: hyanwong

python/tests/test_drawing.py

@@ -1493,12 +1493,13 @@ def test_nonimplemented_base_class(self):
         with pytest.raises(NotImplementedError):
             plot.draw_y_axis(tick_positions=[0])
 
-    def test_nonimplemented_tick_spacing(self):
+    def test_bad_tick_spacing(self):
+        # Integer y_ticks to give auto-generated tick locs is not currently implemented
         t = self.get_binary_tree()
-        with pytest.raises(NotImplementedError):
+        with pytest.raises(TypeError):
             t.draw_svg(y_axis=True, y_ticks=6)
         ts = self.get_simple_ts()
-        with pytest.raises(NotImplementedError):
+        with pytest.raises(TypeError):
             ts.draw_svg(y_axis=True, y_ticks=6)
 
     def test_no_mixed_yscales(self):
@@ -1513,12 +1514,26 @@ def test_draw_defaults(self):
         svg = t.draw_svg()
         self.verify_basic_svg(svg)
 
-    def test_draw_nonbinary(self):
-        t = self.get_nonbinary_tree()
-        svg = t.draw()
-        self.verify_basic_svg(svg)
+    @pytest.mark.parametrize("y_axis", (True, False))
+    @pytest.mark.parametrize("y_label", (True, False))
+    @pytest.mark.parametrize(
+        "tree_height_scale",
+        (
+            "rank",
+            "time",
+        ),
+    )
+    @pytest.mark.parametrize("y_ticks", ([], [0, 1], None))
+    @pytest.mark.parametrize("y_gridlines", (True, False))
+    def test_draw_svg_y_axis_parameter_combos(
+        self, y_axis, y_label, tree_height_scale, y_ticks, y_gridlines
+    ):
+        t = self.get_binary_tree()
         svg = t.draw_svg()
         self.verify_basic_svg(svg)
+        ts = self.get_simple_ts()
+        svg = ts.draw_svg()
+        self.verify_basic_svg(svg, width=200 * ts.num_trees)
 
     def test_draw_multiroot(self):
         t = self.get_multiroot_tree()

python/tskit/drawing.py

@@ -124,16 +124,13 @@ def check_x_scale(x_scale):
 
 def check_ticks(ticks, default_iterable):
     """
-    If y_ticks is iterable, it is a list of tick positions. Otherwise return a default
-    or None if falsey
+    This is trivial, but implemented as a function so that later we can implement a tick
+    locator function, such that e.g. ticks=5 selects ~5 nicely spaced tick locations
+    (ideally with sensible behaviour for log scales)
     """
     if ticks is None:
         return default_iterable
-    try:
-        iter(ticks)
-        return ticks
-    except TypeError:
-        raise NotImplementedError("Autocalculated tick mark locations not implemented.")
+    return ticks
 
 
 def rnd(x):
@@ -341,9 +338,7 @@ class SvgPlot:
     # TODO: we may want to make some of the constants below into parameters
     text_height = 14  # May want to calculate this based on a font size
     line_height = text_height * 1.2  # allowing padding above and below a line
-    root_branch_fraction = (
-        1 / 8
-    )  # Rel. root branch len (unless it has a timed mutation)
+    root_branch_fraction = 1 / 8  # Rel root branch len, unless it has a timed mutation
     default_tick_length = 5
     default_tick_length_site = 10
     # Placement of the axes lines within the padding - not used unless axis is plotted

python/tskit/trees.py

@@ -1631,8 +1631,7 @@ def draw_svg(
             gives no tickmarks). If ``None``, plot one tickmark for each unique
             node value.
         :param bool y_gridlines: Whether to plot horizontal lines behind the tree
-            at each y tickmark. If ``None`` (default), only plot gridlines if a list
-            of ``y_ticks`` has also been given.
+            at each y tickmark.
 
         :return: An SVG representation of a tree.
         :rtype: str
@@ -5395,8 +5394,7 @@ def draw_svg(
             gives no tickmarks). If ``None``, plot one tickmark for each unique
             node value.
         :param bool y_gridlines: Whether to plot horizontal lines behind the tree
-            at each y tickmark. If ``None`` (default), only plot gridlines if a list
-            of ``y_ticks`` has also been given.
+            at each y tickmark.
 
         :return: An SVG representation of a tree sequence.
         :rtype: str