Repository: tskit-dev/msprime (language: python)

Issue:
can't specify population names to `sim_ancestry()` when recapitating
I'm trying to come up with a fix for https://github.com/popsim-consortium/stdpopsim/issues/1140 that will work when SLiM 3.7 arrives, but I'm not sure that the current msprime API provides a (reasonable) way to specify the population names that will be in the `initial_state` population metadata.

* In stdpopsim, we have the name in the `PopulationConfiguration` metadata. But this is metadata, and msprime doesn't look at it.
* We can pass a `population_map` to `Demography.from_old_style()`. This is undocumented. From the source code, it looks like a list of maps---one map per epoch---which is hard to come by unless one does a deep inspection of the epochs for the demography at hand.

Is there something simple to be done here? We have the names; we can give a map from integer id to name. How do we tell msprime what the names are?
can't specify population names to `sim_ancestry()` when recapitating
I'm trying to come up with a fix for https://github.com/popsim-consortium/stdpopsim/issues/1140 that will work when SLiM 3.7 arrives, but I'm not sure that the current msprime API provides a (reasonable) way to specify the population names that will be in the `initial_state` population metadata.

* In stdpopsim, we have the name in the `PopulationConfiguration` metadata. But this is metadata, and msprime doesn't look at it.
* We can pass a `population_map` to `Demography.from_old_style()`. This is undocumented. From the source code, it looks like a list of maps---one map per epoch---which is hard to come by unless one does a deep inspection of the epochs for the demography at hand.

Is there something simple to be done here? We have the names; we can give a map from integer id to name. How do we tell msprime what the names are?


Discussion / hints:
I haven't dug into the details here yet - but is the problem using `from_old_style( )`, or more general?
It's only possible to specify popuation names by passing `demography=msprime.Demography(...)` to `sim_ancestry()`. In stdpopsim, we're using `from_old_style()` to make the Demography object. In the future, we'll convert to Demes demographic models, which will include the name when converting to an `msprime.Demography` (which will probably work great). But in absense of a Demes model (e.g. outside of the future-stdpopsim world) there will be many preexisting codes that want to use `sim_ancestry()` in order to recapitate, and their only option will be via `from_old_style()` - but the population_map is undocumented and even if it get documented I don't think its a very nice path to mapping ids to names for the legacy-api recapitation use case.

So, really, this issue is about coming up with some canonical approach to match up population names when doing recaptitation if one has legacy-api code.
I think the most common case will be SLiM recapitation with a single root population that needs recaptitation. This also needs a population name or recaptitation fails, right?
PS: I've been ignoring the `pyslim` api in my recent investigations because this seems to want to convert the tree sequence to an old version upon loading. I don't know what's going on here, I kinda assumed you (@petrelharp) hadn't updated things yet (no pressure :)).
What if we added a ``name`` attribute to the 0.x [PopulationConfiguration](https://github.com/tskit-dev/msprime/blob/c034a5d24a39ad888a603a2c2042298ad0f8265f/msprime/demography.py#L2822) class @grahamgower, and used this (if set) in the [_from_old_style_simple](https://github.com/tskit-dev/msprime/blob/c034a5d24a39ad888a603a2c2042298ad0f8265f/msprime/demography.py#L1809) code path? Or, maybe it would be better to inspect the metadata to see if a "name" key is present? (That would be the simplest, if it worked I think)

I think it would be simpler to pass a `population_names` parameter to `from_old_style()`, which is just a list of parameter names in the appropriate order.
OK, I'll do this today. I want to get a release out today, so it's a quick turnaround I'm afraid - it'd be great if I could get your review a bit later.
I haven't dug into the details here yet - but is the problem using `from_old_style( )`, or more general?
It's only possible to specify popuation names by passing `demography=msprime.Demography(...)` to `sim_ancestry()`. In stdpopsim, we're using `from_old_style()` to make the Demography object. In the future, we'll convert to Demes demographic models, which will include the name when converting to an `msprime.Demography` (which will probably work great). But in absense of a Demes model (e.g. outside of the future-stdpopsim world) there will be many preexisting codes that want to use `sim_ancestry()` in order to recapitate, and their only option will be via `from_old_style()` - but the population_map is undocumented and even if it get documented I don't think its a very nice path to mapping ids to names for the legacy-api recapitation use case.

So, really, this issue is about coming up with some canonical approach to match up population names when doing recaptitation if one has legacy-api code.
I think the most common case will be SLiM recapitation with a single root population that needs recaptitation. This also needs a population name or recaptitation fails, right?
PS: I've been ignoring the `pyslim` api in my recent investigations because this seems to want to convert the tree sequence to an old version upon loading. I don't know what's going on here, I kinda assumed you (@petrelharp) hadn't updated things yet (no pressure :)).
What if we added a ``name`` attribute to the 0.x [PopulationConfiguration](https://github.com/tskit-dev/msprime/blob/c034a5d24a39ad888a603a2c2042298ad0f8265f/msprime/demography.py#L2822) class @grahamgower, and used this (if set) in the [_from_old_style_simple](https://github.com/tskit-dev/msprime/blob/c034a5d24a39ad888a603a2c2042298ad0f8265f/msprime/demography.py#L1809) code path? Or, maybe it would be better to inspect the metadata to see if a "name" key is present? (That would be the simplest, if it worked I think)

I think it would be simpler to pass a `population_names` parameter to `from_old_style()`, which is just a list of parameter names in the appropriate order.
OK, I'll do this today. I want to get a release out today, so it's a quick turnaround I'm afraid - it'd be great if I could get your review a bit later.

Fix patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index df6aff98a..dc9e46071 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,5 +1,7 @@
 # Changelog
 
+**New features**:
+
 
 **Bug fixes**:
 
@@ -8,6 +10,14 @@
   does not support).
   ({pr}`1938`, {issue}`1937`, {user}`grahamgower`).
 
+- Do not assume Population metadata schemas contain the ``properties``
+  and ``additionalProperties`` attributes ({issue}`1947`, {pr}`1954`,
+  {user}`jeromekelleher`).
+
+- Read the population name from PopulationConfiguration ``metadata`` in
+  ``Demography.from_old_style`` ({issue}`1950`, {pr}`1954`,
+  {user}`jeromekelleher`)
+
 ## [1.0.4] - 2021-12-01
 
 **New features**:
diff --git a/msprime/demography.py b/msprime/demography.py
index 05831d891..1a722fef7 100644
--- a/msprime/demography.py
+++ b/msprime/demography.py
@@ -1132,8 +1132,8 @@ def insert_extra_populations(self, tables):
         # the schema here, but there's none available right now.
         schema = tables.populations.metadata_schema.schema
         if schema is not None:
-            properties = schema["properties"]
-            additional_properties = schema["additionalProperties"]
+            properties = schema.get("properties", {})
+            additional_properties = schema.get("additionalProperties", True)
             name_in_metadata = "name" in properties or additional_properties
             description_in_metadata = (
                 "description" in properties or additional_properties
@@ -1817,7 +1817,11 @@ def _from_old_style_simple(
         """
         demography = Demography()
         for pop_config in population_configurations:
-            demography._add_population_from_old_style(pop_config)
+            name = None
+            if pop_config.metadata is not None:
+                # If there's a name defined in the old-style metadata use that
+                name = pop_config.metadata.get("name", None)
+            demography._add_population_from_old_style(pop_config, name)
         if migration_matrix is not None:
             demography.migration_matrix = np.array(migration_matrix)
         if demographic_events is not None:
@@ -1847,6 +1851,14 @@ def from_old_style(
         not be performed and the sample sizes specified in the old-style
         :class:`.PopulationConfiguration` objects will be ignored.
 
+        Each :class:`.PopulationConfiguration` instance in the list of
+        ``population_configurations`` corresponds to the equivalent
+        :class:`.Population` object in the returned :class:`.Demography`.
+        If a PopulationConfiguration has ``metadata`` defined and this
+        dictionary contains a ``name`` field, this will be used as the
+        :class:`.Population` name. Otherwise, the default population
+        names will be used.
+
         Please see the :ref:`sec_ancestry_samples` section for details on
         how to specify sample locations in :func:`.sim_ancestry`.
 


Test patch:
diff --git a/tests/test_demography.py b/tests/test_demography.py
index 785c86dc1..16ce0f05a 100644
--- a/tests/test_demography.py
+++ b/tests/test_demography.py
@@ -3300,7 +3300,7 @@ def test_default(self):
     def test_errors(self):
         for bad_metadata in [b"asdf", Exception]:
             pop_conf = msprime.PopulationConfiguration(2, metadata=bad_metadata)
-            with pytest.raises(TypeError):
+            with pytest.raises(AttributeError):
                 msprime.simulate(population_configurations=[pop_conf])
 
     def test_multi_population(self):
@@ -4733,6 +4733,24 @@ def test_defaults(self):
         assert list(demog.migration_matrix) == [[0]]
         assert list(demog.events) == []
 
+    def test_names_in_metadata(self):
+        pop_configs = [
+            msprime.PopulationConfiguration(metadata={"name": "A"}),
+            msprime.PopulationConfiguration(metadata={"name": "B"}),
+        ]
+        demog = msprime.Demography.from_old_style(population_configurations=pop_configs)
+        assert demog.populations[0].name == "A"
+        assert demog.populations[1].name == "B"
+
+    def test_names_in_metadata_mixed(self):
+        pop_configs = [
+            msprime.PopulationConfiguration(),
+            msprime.PopulationConfiguration(metadata={"name": "B"}),
+        ]
+        demog = msprime.Demography.from_old_style(population_configurations=pop_configs)
+        assert demog.populations[0].name == "pop_0"
+        assert demog.populations[1].name == "B"
+
     def test_pop_configs_defaults(self):
         for n in range(1, 5):
             pop_configs = [msprime.PopulationConfiguration() for _ in range(n)]
diff --git a/tests/test_simulate_from.py b/tests/test_simulate_from.py
index a517ba282..afaa60cde 100644
--- a/tests/test_simulate_from.py
+++ b/tests/test_simulate_from.py
@@ -1055,6 +1055,20 @@ def test_extra_pops_missing_description_metadata(self):
         assert ts.population(0).metadata == {"name": "X"}
         assert ts.population(1).metadata == {"name": "Z"}
 
+    def test_extra_pops_minimal_schema(self):
+        tables = tskit.TableCollection(1)
+        tables.populations.metadata_schema = tskit.MetadataSchema.permissive_json()
+        tables.populations.add_row(metadata={"name": "X"})
+        tables.nodes.add_row(flags=tskit.NODE_IS_SAMPLE, population=0)
+        tables.nodes.add_row(flags=tskit.NODE_IS_SAMPLE, population=0)
+        d = msprime.Demography.from_tree_sequence(
+            tables.tree_sequence(), initial_size=1
+        )
+        d.add_population(name="Z", description="ZZ", initial_size=1)
+        ts = msprime.sim_ancestry(initial_state=tables, demography=d, random_seed=1)
+        assert ts.population(0).metadata == {"name": "X"}
+        assert ts.population(1).metadata == {"name": "Z", "description": "ZZ"}
+
     def test_extra_pops_missing_extra_metadata(self):
         tables = tskit.TableCollection(1)
         tables.populations.metadata_schema = tskit.MetadataSchema(