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

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 =====================