Add Graph.change_time_units

[molpopgen]

Closes #574

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.81%. Comparing base (e904fd6) to head (9253e75).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #599   +/-   ##
=======================================
  Coverage   99.81%   99.81%           
=======================================
  Files           5        5           
  Lines        1600     1619   +19     
=======================================
+ Hits         1597     1616   +19     
  Misses          3        3           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[grahamgower]

Does this need to convert into generations first? I reckon it does the wrong thing when graph.time_units != "generations" and you call graph.change_time_units() with time_units != "generations". E.g.

graph1 = demes.load(...)  # in generations.
graph2 = graph1.change_time_units("years", 25)
graph3 = graph2.change_time_units("months", 25*12)

Will effectively produce times in graph3 multiplied by 25*25*12 instead of 25*12.

[grahamgower]

Doesn't this also do the wrong thing when graph.time_units != "generations" and you call graph.change_time_units("generations", 1)? Won't it just set graph.time_units = "generations", while keeping the time values the same?

I think in this case it would be better to just return self.in_generations(). And probably enforce that the time_units passed should be 1 (any other value suggests that the caller is somehow making a mistake - possibly misunderstanding the purpose of the arguments).

[molpopgen]

I think I got both of your questions answered. Unfortunately, the current tests were invariant to the current vs the previous implementation, implying that they are poor tests.

I'll add a double-convert test like you mentioned...

[grahamgower]

The test still looks very fragile to me. There's nothing here checking that the times or the units have actually changed. For instance, one can just delete most of the code in change_time_units() and the tests still pass.

diff --git a/demes/demes.py b/demes/demes.py
index d965a7d..1db085a 100644
--- a/demes/demes.py
+++ b/demes/demes.py
@@ -2017,19 +2017,6 @@ class Graph:
                 raise ValueError(f"generation time must be 1, got {generation_time}")
             return graph
 
-        for deme in graph.demes:
-            deme.start_time *= generation_time
-            for epoch in deme.epochs:
-                epoch.start_time *= generation_time
-                epoch.end_time *= generation_time
-        for migration in graph.migrations:
-            migration.start_time *= generation_time
-            migration.end_time *= generation_time
-        for pulse in graph.pulses:
-            pulse.time *= generation_time
-        graph.time_units = time_units
-        graph.generation_time = generation_time
-
         return graph
 
     def rename_demes(self, names: Mapping[str, str]) -> Graph:

$ uv run python -m pytest tests/test_demes.py 
===================== test session starts =====================
platform linux -- Python 3.11.10, pytest-8.3.3, pluggy-1.6.0
rootdir: /home/grg/src/demes/demes-python
configfile: pyproject.toml
plugins: xdist-3.8.0, cov-7.0.0
collected 250 items                                           

tests/test_demes.py ................................... [ 14%]
....................................................... [ 36%]
....................................................... [ 58%]
....................................................... [ 80%]
..................................................      [100%]

===================== 250 passed in 0.33s =====================

[molpopgen]

The test still looks very fragile to me. There's nothing here checking that the times or the units have actually changed. For instance, one can just delete most of the code in change_time_units() and the tests still pass.

diff --git a/demes/demes.py b/demes/demes.py
index d965a7d..1db085a 100644
--- a/demes/demes.py
+++ b/demes/demes.py
@@ -2017,19 +2017,6 @@ class Graph:
                 raise ValueError(f"generation time must be 1, got {generation_time}")
             return graph
 
-        for deme in graph.demes:
-            deme.start_time *= generation_time
-            for epoch in deme.epochs:
-                epoch.start_time *= generation_time
-                epoch.end_time *= generation_time
-        for migration in graph.migrations:
-            migration.start_time *= generation_time
-            migration.end_time *= generation_time
-        for pulse in graph.pulses:
-            pulse.time *= generation_time
-        graph.time_units = time_units
-        graph.generation_time = generation_time
-
         return graph
 
     def rename_demes(self, names: Mapping[str, str]) -> Graph:

$ uv run python -m pytest tests/test_demes.py 
===================== test session starts =====================
platform linux -- Python 3.11.10, pytest-8.3.3, pluggy-1.6.0
rootdir: /home/grg/src/demes/demes-python
configfile: pyproject.toml
plugins: xdist-3.8.0, cov-7.0.0
collected 250 items                                           

tests/test_demes.py ................................... [ 14%]
....................................................... [ 36%]
....................................................... [ 58%]
....................................................... [ 80%]
..................................................      [100%]

===================== 250 passed in 0.33s =====================

Can you point me to relevant tests that you were using in the implementation over in stdpopsim?

[molpopgen]

The mypy failure is wrong and cannot be reproduced locally with either of the following:

uv sync -p 3.14 --all-groups
uv run mypy

uv sync -p 3.12 --all-groups
uv run mypy

[grahamgower]

Can you point me to relevant tests that you were using in the implementation over in stdpopsim?

There probably weren't any tests there. If I recall correctly, the code there was not intended to be merged, it was just there as part of a conversion of models to demes YAML, and the final converted models were to be merged (eventually). The code was in the in-dev PR to help document the semi-automated conversion process. The PR was incomplete and never got merged.

If you're ok with it, I can add a commit or two with additional tests. It won't be today though.

[molpopgen]

Can you point me to relevant tests that you were using in the implementation over in stdpopsim?

There probably weren't any tests there. If I recall correctly, the code there was not intended to be merged, it was just there as part of a conversion of models to demes YAML, and the final converted models were to be merged (eventually). The code was in the in-dev PR to help document the semi-automated conversion process. The PR was incomplete and never got merged.

If you're ok with it, I can add a commit or two with additional tests. It won't be today though.

Sure -- I'm okay with that. I did add a locally scoped function to ensure that the values do indeed change.