Repository: tskit-dev/tskit

Issue:
Local variable referenced before assignment when drawing SVG
I am calling the `draw_svg` method from a `Tree` object and the root node's time is too far back, so I am trying to use the `max_time` option to draw only the relevant part. However, the method ends up giving an `UnboundLocalError`:

`UnboundLocalError: local variable 'max_node_height' referenced before assignment`

Discussion hints:
benjeffery: @hyanwong This seems to be the obvious fix, but can I get a review to check?
codecov[bot]: ## [Codecov](https://app.codecov.io/gh/tskit-dev/tskit/pull/3274?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=tskit-dev) Report
:x: Patch coverage is `0%` with `1 line` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 78.89%. Comparing base ([`6f6aa73`](https://app.codecov.io/gh/tskit-dev/tskit/commit/6f6aa73e3b7ed49764262054eb7a65958367c90b?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=tskit-dev)) to head ([`9fbbb08`](https://app.codecov.io/gh/tskit-dev/tskit/commit/9fbbb08907068e2989f7ece1ba8f29ff2f3782ca?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=tskit-dev)).
:warning: Report is 1 commits behind head on main.

| [Files with missing lines](https://app.codecov.io/gh/tskit-dev/tskit/pull/3274?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=tskit-dev) | Patch % | Lines |
|---|---|---|
| [python/tskit/drawing.py](https://app.codecov.io/gh/tskit-dev/tskit/pull/3274?src=pr&el=tree&filepath=python%2Ftskit%2Fdrawing.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=tskit-dev#diff-cHl0aG9uL3Rza2l0L2RyYXdpbmcucHk=) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/tskit-dev/tskit/pull/3274?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=tskit-dev) |
> :exclamation:  There is a different number of reports uploaded between BASE (6f6aa73) and HEAD (9fbbb08). Click for more details.
> 
> <details><summary>HEAD has 22 uploads less than BASE</summary>
>
>| Flag | BASE (6f6aa73) | HEAD (9fbbb08) |
>|------|------|------|
>|python-tests-no-jit|12|6|
>|c-tests|2|1|
>|python-tests-numpy1|2|1|
>|lwt-tests|2|1|
>|python-c-tests|2|1|
>|python-tests|12|0|
></details>

<details><summary>Additional details and impacted files</summary>



```diff
@@             Coverage Diff             @@
##             main    #3274       +/-   ##
===========================================
- Coverage   89.75%   78.89%   -10.87%     
===========================================
  Files          29       29               
  Lines       32545    32546        +1     
  Branches     5962     5962               
===========================================
- Hits        29211    25677     -3534     
- Misses       1890     5323     +3433     
- Partials     1444     1546      +102     
```

| [Flag](https://app.codecov.io/gh/tskit-dev/tskit/pull/3274/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=tskit-dev) | Coverage Δ | |
|---|---|---|
| [c-tests](https://app.codecov.io/gh/tskit-dev/tskit/pull/3274/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=tskit-dev) | `86.76% <ø> (ø)` | |
| [lwt-tests](https://app.codecov.io/gh/tskit-dev/tskit/pull/3274/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=tskit-dev) | `80.38% <ø> (ø)` | |
| [python-c-tests](https://app.codecov.io/gh/tskit-dev/tskit/pull/3274/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=tskit-dev) | `88.24% <ø> (ø)` | |
| [python-tests](https://app.codecov.io/gh/tskit-dev/tskit/pull/3274/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=tskit-dev) | `?` | |
| [python-tests-no-jit](https://app.codecov.io/gh/tskit-dev/tskit/pull/3274/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=tskit-dev) | `33.02% <0.00%> (-0.01%)` | :arrow_down: |
| [python-tests-numpy1](https://app.codecov.io/gh/tskit-dev/tskit/pull/3274/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=tskit-dev) | `50.70% <0.00%> (-0.01%)` | :arrow_down: |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=tskit-dev#carryforward-flags-in-the-pull-request-comment) to find out more.

| [Files with missing lines](https://app.codecov.io/gh/tskit-dev/tskit/pull/3274?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=tskit-dev) | Coverage Δ | |
|---|---|---|
| [python/tskit/drawing.py](https://app.codecov.io/gh/tskit-dev/tskit/pull/3274?src=pr&el=tree&filepath=python%2Ftskit%2Fdrawing.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=tskit-dev#diff-cHl0aG9uL3Rza2l0L2RyYXdpbmcucHk=) | `22.69% <0.00%> (-75.71%)` | :arrow_down: |

... and [12 files with indirect coverage changes](https://app.codecov.io/gh/tskit-dev/tskit/pull/3274/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=tskit-dev)
</details>
<details><summary> :rocket: New features to boost your workflow: </summary>

- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
</details>

Solution patch:
diff --git a/python/CHANGELOG.rst b/python/CHANGELOG.rst
index 8817be18..584f6ece 100644
--- a/python/CHANGELOG.rst
+++ b/python/CHANGELOG.rst
@@ -51,7 +51,11 @@
   "self" comparisons for `TreeSequence.genetic_relatedness`. This changes the
   error code returned in some situations.
   (:user:`andrewkern`, :user:`petrelharp`, :pr:`3235`, :issue:`3055`)
-  
+
+- Fix ``UnboundLocalError`` in ``draw_svg()`` when using numeric ``max_time``
+  values with mutations over roots.
+  (:user:`benjeffery`, :pr:`3274`, :issue:`3273`)
+
 
 **Breaking changes** 
 
diff --git a/python/tskit/drawing.py b/python/tskit/drawing.py
index 64e3aaef..ad0cfb95 100644
--- a/python/tskit/drawing.py
+++ b/python/tskit/drawing.py
@@ -2003,6 +2003,8 @@ class SvgTree(SvgAxisPlot):
                 max_node_height = self.ts.max_root_time
                 max_mut_height = np.nanmax(np.append(mut_time, 0))
                 max_time = max(max_node_height, max_mut_height)  # Reuse variable
+            else:
+                max_node_height = max(self.node_height.values())
             if min_time == "tree":
                 min_time = min(self.node_height.values())
                 # don't need to check mutation times, as they must be above a node


Test patch:
diff --git a/python/tests/test_drawing.py b/python/tests/test_drawing.py
index ce13a64c..15ec21e4 100644
--- a/python/tests/test_drawing.py
+++ b/python/tests/test_drawing.py
@@ -1868,6 +1868,22 @@ class TestDrawSvg(TestDrawSvgBase):
         svg3 = ts.draw_svg(min_time=min(ts.tables.nodes.time), y_axis=True)
         assert svg2 == svg3
 
+    def test_numeric_max_time_with_mutations_over_roots(self):
+        t = self.get_mutations_over_roots_tree()
+        max_time_value = 10.0  # Use a numeric max_time value
+
+        with pytest.warns(
+            UserWarning, match="Mutations .* are above nodes which are not present"
+        ):
+            svg = t.draw_svg(max_time=max_time_value)
+        self.verify_basic_svg(svg)
+
+        with pytest.warns(
+            UserWarning, match="Mutations .* are above nodes which are not present"
+        ):
+            svg_log = t.draw_svg(max_time=max_time_value, time_scale="log_time")
+        self.verify_basic_svg(svg_log)
+
     #
     # TODO: update the tests below here to check the new SVG based interface.
     #