Repository: tskit-dev/tsdate

Issue:
Store PopulationSizeHistory as a json list
It would be nice to store the passed-in PopulationSizeHistory object in the provenance of a tsdated tree sequence. I don't think it is a very large object, right? It just consists of 2 arrays, neither of which is likely to be millions of numbers long. Any reason why not to store it?

Discussion:
hyanwong: Does this look right @nspope ?

Also pinging @benjeffery , because I have previously wondered about the general strategy for encoding simple objects into the `tskit` provenance information, and this is one possibility, although it does involve allowing multiple types for a parameter (i.e. either a simple object of the correct type or a dictionary of parameters used to instantiate an object of the correct type). The case I was previously stuck on was whether/how to encode a recombination rate map into tsinfer provenance information. That's potentially a much bigger array, though, so maybe not worth it. See https://github.com/tskit-dev/tsinfer/issues/332#issuecomment-867792691

Patch:
diff --git a/tsdate/core.py b/tsdate/core.py
index 4bdfddb..c007b00 100644
--- a/tsdate/core.py
+++ b/tsdate/core.py
@@ -39,6 +39,7 @@ from tqdm import tqdm
 
 from . import approx
 from . import base
+from . import demography
 from . import prior
 from . import provenance
 
@@ -1169,10 +1170,12 @@ def date(
         one whose non-sample nodes are undated.
     :param PopulationSizeHistory population_size: The estimated (diploid) effective
         population size used to construct the (default) conditional coalescent
-        prior. This may be a single value (for a population with constant size), or
-        a :class:`PopulationSizeHistory` object (for a population with time-varying
-        size). This is used when ``priors`` is ``None``.  Conversely, if ``priors``
-        is not ``None``, no ``population_size`` value should be given.
+        prior. For a population with constant size, this can be given as a single
+        value. For a population with time-varying size, this can be given directly as
+        a :class:`PopulationSizeHistory` object or a parameter dictionary passed
+        to initialise a class:`PopulationSizeHistory` object. This is used when
+        ``priors`` is ``None``.  Conversely, if ``priors`` is not ``None``, no
+        ``population_size`` value should be specified.
     :param float mutation_rate: The estimated mutation rate per unit of genome per
         unit time. If provided, the dating algorithm will use a mutation rate clock to
         help estimate node dates. Default: ``None``
@@ -1226,6 +1229,10 @@ def date(
             )
         else:
             population_size = Ne
+
+    if isinstance(population_size, dict):
+        population_size = demography.PopulationSizeHistory(**population_size)
+
     if method == "variational_gamma":
         tree_sequence, dates, posteriors, timepoints, eps, nds = variational_dates(
             tree_sequence,
@@ -1261,8 +1268,8 @@ def date(
     )
     if isinstance(population_size, (int, float)):
         params["population_size"] = population_size
-    else:
-        params["population_size"] = "TODO: PopulationSizeHistory object"
+    elif isinstance(population_size, demography.PopulationSizeHistory):
+        params["population_size"] = population_size.as_dict()
     provenance.record_provenance(
         tables,
         "date",
diff --git a/tsdate/demography.py b/tsdate/demography.py
index 57f7885..47324d5 100644
--- a/tsdate/demography.py
+++ b/tsdate/demography.py
@@ -69,39 +69,48 @@ class PopulationSizeHistory:
 
     def __init__(self, population_size, time_breaks=None):
         """
-        :param np.ndarray population_size: A numpy array containing diploid
+        :param array_like population_size: An array containing diploid
             population sizes per epoch
-        :param np.ndarray time_breaks: A sorted numpy array containing time
+        :param array_like time_breaks: A sorted array containing time
             breaks that divide epochs, measured in units of generations in the
             past
         """
 
         if time_breaks is None:
-            time_breaks = np.array([], dtype=float)
+            time_breaks = []
 
         if isinstance(population_size, (int, float)):
-            if not population_size > 0:
-                raise ValueError("Population size must be greater than 0")
             population_size = np.array([population_size], dtype=float)
         else:
-            if not isinstance(population_size, np.ndarray):
-                raise ValueError("Population sizes must be in a numpy array")
-            if not np.all(population_size > 0.0):
-                raise ValueError("Population sizes must be greater than 0")
-            if not isinstance(time_breaks, np.ndarray):
-                raise ValueError("Epoch time breaks must be in a numpy array")
-            if not time_breaks.size == population_size.size - 1:
+            try:
+                population_size = np.array(population_size, dtype=float)
+            except (ValueError, TypeError) as e:
+                raise e.__class__(
+                    "Population sizes must be convertable to a numpy float array"
+                ) from e
+        if not np.all(population_size > 0.0):
+            raise ValueError("Population sizes must be greater than 0")
+        if not np.all(np.isfinite(population_size)):
+            raise ValueError("Population sizes must be finite")
+
+        try:
+            time_breaks = np.array(time_breaks, dtype=float)
+        except (ValueError, TypeError) as e:
+            raise e.__class__(
+                "Time breaks must be convertable to a numpy float array"
+            ) from e
+        if not time_breaks.size == population_size.size - 1:
+            raise ValueError(
+                "The length of the population size array must be one less "
+                "than the number of epoch time breaks"
+            )
+        if time_breaks.size > 0:
+            if not np.all(time_breaks > 0.0):
+                raise ValueError("Epoch time breaks must be greater than 0")
+            if not np.all(np.diff(time_breaks) > 0.0):
                 raise ValueError(
-                    "The length of the population size array must be one less "
-                    "than the number of epoch time breaks"
+                    "Epoch time breaks must be unique and in increasing order"
                 )
-            if time_breaks.size > 0:
-                if not np.all(time_breaks > 0.0):
-                    raise ValueError("Epoch time breaks must be greater than 0")
-                if not np.all(np.diff(time_breaks) > 0.0):
-                    raise ValueError(
-                        "Epoch time breaks must be unique and in increasing order"
-                    )
 
         self.time_breaks = np.append([0.0], time_breaks.flatten())
         self.population_size = 2 * population_size.flatten()
@@ -111,6 +120,17 @@ class PopulationSizeHistory:
         self.coalescent_breaks = coalescent_breaks
         self.coalescent_rate = coalescent_rate
 
+    def as_dict(self):
+        """
+        Return the population size history as a dictionary of parameters
+        that can be used to initialise a new object
+        """
+        ret_val = {"population_size": list(self.population_size / 2)}
+        assert self.time_breaks[0] == 0.0
+        if len(self.time_breaks) > 1:
+            ret_val["time_breaks"] = list(self.time_breaks[1:])
+        return ret_val
+
     def to_natural_timescale(self, coalescent_time_ago):
         """
         Convert a vector of times from coalescent units to generations


Test patch:
diff --git a/tests/test_functions.py b/tests/test_functions.py
index fafca92..b6aa798 100644
--- a/tests/test_functions.py
+++ b/tests/test_functions.py
@@ -2147,16 +2147,12 @@ class TestPopulationSizeHistory:
             assert np.isclose(x, gens[i])
 
     def test_to_coalescent_timescale(self):
-        demography = PopulationSizeHistory(
-            np.array([1000, 2000, 3000]), np.array([500, 2500])
-        )
+        demography = PopulationSizeHistory([1000, 2000, 3000], [500, 2500])
         coaltime = demography.to_coalescent_timescale(np.array([250, 1500]))
         assert np.allclose(coaltime, [0.125, 0.5])
 
     def test_to_natural_timescale(self):
-        demography = PopulationSizeHistory(
-            np.array([1000, 2000, 3000]), np.array([500, 2500])
-        )
+        demography = PopulationSizeHistory([1000, 2000, 3000], [500, 2500])
         time = demography.to_natural_timescale(np.array([0.125, 0.5]))
         assert np.allclose(time, [250, 1500])
 
@@ -2169,9 +2165,7 @@ class TestPopulationSizeHistory:
     def test_moments_numerically(self):
         alpha = 2.8
         beta = 1.7
-        demography = PopulationSizeHistory(
-            np.array([1000, 2000, 3000]), np.array([500, 2500])
-        )
+        demography = PopulationSizeHistory([1000, 2000, 3000], [500, 2500])
         numer_mn, _ = scipy.integrate.quad(
             lambda t: demography.to_natural_timescale(np.array([t]))
             * scipy.stats.gamma.pdf(t, alpha, scale=1 / beta),
@@ -2192,10 +2186,16 @@ class TestPopulationSizeHistory:
         assert np.isclose(numer_va, analy_va)
 
     def test_bad_arguments(self):
-        with pytest.raises(ValueError, match="a numpy array"):
-            PopulationSizeHistory([1])
-        with pytest.raises(ValueError, match="a numpy array"):
-            PopulationSizeHistory(np.array([1, 1]), [1])
+        with pytest.raises(ValueError, match="greater than 0"):
+            PopulationSizeHistory([None])
+        with pytest.raises(ValueError, match="finite"):
+            PopulationSizeHistory(np.inf)
+        with pytest.raises(ValueError, match="a numpy float array"):
+            PopulationSizeHistory(["a"])
+        with pytest.raises(TypeError, match="a numpy float array"):
+            PopulationSizeHistory([{}])
+        with pytest.raises(TypeError, match="a numpy float array"):
+            PopulationSizeHistory(np.array([1, 1]), [{}])
         with pytest.raises(ValueError, match="must be greater than 0"):
             PopulationSizeHistory(0)
         with pytest.raises(ValueError, match="must be greater than 0"):
diff --git a/tests/test_provenance.py b/tests/test_provenance.py
index 93dad9e..15b386d 100644
--- a/tests/test_provenance.py
+++ b/tests/test_provenance.py
@@ -54,26 +54,29 @@ class TestProvenance:
         assert np.isclose(rec["parameters"]["mutation_rate"], mu)
         assert np.isclose(rec["parameters"]["population_size"], Ne)
 
-    @pytest.mark.skip(
-        reason="Not implemented yet: https://github.com/tskit-dev/tsdate/issues/274"
+    @pytest.mark.parametrize(
+        "popdict",
+        [
+            {"population_size": [1, 2, 3], "time_breaks": [1, 1.2]},
+            {"population_size": [123]},
+        ],
     )
-    def test_date_popsizehist_recorded(self):
+    def test_date_popsizehist_recorded(self, popdict):
         ts = utility_functions.single_tree_ts_n2()
         mu = 0.123
-        popsize = tsdate.demography.PopulationSizeHistory(
-            np.array([1, 1]), np.array([1])
-        )
-        dated_ts = tsdate.date(ts, population_size=popsize, mutation_rate=mu)
-        rec = json.loads(dated_ts.provenance(-1).record)
-        assert np.isclose(rec["parameters"]["mutation_rate"], mu)
-        assert "population_size" in rec["parameters"]
-        # TODO: check that the population size history is recorded correctly
-        # https://github.com/tskit-dev/tsdate/issues/274
-        assert np.isclose(
-            # This is wrong - left in for now to show the sort of thing we want
-            (np.array(rec["parameters"]["population_size"])),
-            popsize,
-        )
+        for use_class in (False, True):
+            if use_class:
+                popsize = tsdate.demography.PopulationSizeHistory(**popdict)
+            else:
+                popsize = popdict
+            dated_ts = tsdate.date(ts, population_size=popsize, mutation_rate=mu)
+            rec = json.loads(dated_ts.provenance(-1).record)
+            assert np.isclose(rec["parameters"]["mutation_rate"], mu)
+            assert "population_size" in rec["parameters"]
+            popsz = rec["parameters"]["population_size"]
+            assert len(popsz) == len(popdict)
+            for param, val in popdict.items():
+                assert np.all(np.isclose(val, popsz[param]))
 
     def test_preprocess_cmd_recorded(self):
         ts = utility_functions.ts_w_data_desert(40, 60, 100)