Renaming demes within Graph object

[aabiddanda]

I encountered a need for this feature when looking to develop a snakemake workflow that expected specific deme labels for post-processing of simulations. The method is fairly straightforward and implements the type-checks. I will update the tests for this as well if it is a useful feature for others to have or if alternative spec-checks should be performed.

[grahamgower]

Hi @aabiddanda! This is a good idea, so I agree we need to export an API function for this. And I think your choice to put this at demes.Graph.rename_demes() is a good one. Actually, there's already an implementation of this function hiding in the ms conversion code (see below). Perhaps you'd like to move the existing code and make the ms converter use the new function instead? The assert should be changed to raise a ValueError instead, and the function probably needs its own set of tests (it looks like I was a bit lazy with the existing tests, and just tested the function that calls the renaming code).

demes-python/demes/ms.py

Lines 824 to 839 in 392c6a0

	def remap_deme_names(graph: demes.Graph, names: Mapping[str, str]) -> demes.Graph:
	assert sorted(names.keys()) == sorted(deme.name for deme in graph.demes)
	graph = copy.deepcopy(graph)
	for deme in graph.demes:
	deme.name = names[deme.name]
	deme.ancestors = [names[ancestor] for ancestor in deme.ancestors]
	for migration in graph.migrations:
	migration.source = names[migration.source]
	migration.dest = names[migration.dest]
	for pulse in graph.pulses:
	pulse.sources = [names[s] for s in pulse.sources]
	pulse.dest = names[pulse.dest]
	for k, deme in list(graph._deme_map.items()):
	del graph._deme_map[k]
	graph._deme_map[names[k]] = deme
	return graph

[codecov]

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (392c6a0) 99.81% compared to head (8339d52) 99.81%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #499   +/-   ##
=======================================
  Coverage   99.81%   99.81%           
=======================================
  Files           5        5           
  Lines        1585     1595   +10     
=======================================
+ Hits         1582     1592   +10     
  Misses          3        3           

Impacted Files	Coverage Δ
demes/demes.py	100.00% <100.00%> (ø)
demes/ms.py	100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[aabiddanda]

@grahamgower great spot on this lurking already in the codebase! I'll move this code to the appropriate place and implement some tests explicitly targeting this functionality to pass codecov shortly.

[grahamgower]

Thanks @aabiddanda, this is looking good. Would you be able to expand the tests to include the nominal cases too? I.e. check the deme names after renaming are what the should be, for each of the deme names, ancestors, migration dest/source, pulse dest/source.

[grahamgower]

I don't think its necessary to do explicit type checking here. If a non-dict is passed, this function will fail anyhow with an indexing error of some sort.

[grahamgower]

What's the reasoning for this being a MutableMapping rather than just a Mapping? This function shouldn't be modifying the mapping, right?

[aabiddanda]

The reasoning for the MutableMapping was for the scenario where you only want to change the label of a single deme but not mess with all of the other demes (specifically the ancestral demes). The basis is that if an existing deme name does not exist you may want to default to keeping that name by adding it to the renaming mapping (see code below).

https://github.com/aabiddanda/demes-python/blob/e2f6eb1fd78dab44045daa080add5f17bdcaf5d9/demes/demes.py#L2013-L2015

This relaxes the requirement that every deme needs to be defined in the names dictionary as well. If you think that adding in conditional statements to the cases to only rename demes that are named (rather than all demes in the graph) I can certainly implement that if its more in keeping with the spec.

[grahamgower]

Yes, renaming only a subset of the demes seems reasonable, but I don't think we should be mutating the argument here. Using conditionals would be fine, but if you want to keep the code as it is, then you could copy the names dict at the start of the function.

[aabiddanda]

@grahamgower should the changelog be changed as well? I don't think that this necessarily constitutes a new version per-se but to maintain that it is a new feature in a to-be-released version? Let me know if this would be easier to handle closer to when you are thinking about releasing another version.

[grahamgower]

This looks great!

[grahamgower]

Don't worry about the changelog - I'll update that when I do the next release.