search

popsim-consortium / demes-python

Tools for describing and manipulating demographic models.
https://popsim-consortium.github.io/demes-docs/
ISC License
18 stars 6 forks source link

Renaming demes within Graph object #499

Closed aabiddanda closed 1 year ago

aabiddanda commented 1 year ago

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 commented 1 year ago

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).

https://github.com/popsim-consortium/demes-python/blob/392c6a0eb5e70223a00d6659df2134317a94bdf0/demes/ms.py#L824-L839

codecov[bot] commented 1 year ago

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 ```diff @@ 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](https://codecov.io/gh/popsim-consortium/demes-python/pull/499?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=popsim-consortium) | Coverage Δ | | |---|---|---| | [demes/demes.py](https://codecov.io/gh/popsim-consortium/demes-python/pull/499?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=popsim-consortium#diff-ZGVtZXMvZGVtZXMucHk=) | `100.00% <100.00%> (ø)` | | | [demes/ms.py](https://codecov.io/gh/popsim-consortium/demes-python/pull/499?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=popsim-consortium#diff-ZGVtZXMvbXMucHk=) | `100.00% <100.00%> (ø)` | | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=popsim-consortium). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=popsim-consortium)

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

aabiddanda commented 1 year ago

@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.

aabiddanda commented 1 year ago

@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 commented 1 year ago

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