census node per lineage, not segment: closes #2540

[petrelharp]

I think this was a conceptual error: all segments in a given lineage at a given time point in the simulation are in the same genome (=node=haplosome), and hence in doing a census event we should assign a single node to each lineage, not each segment.

If we agree, this needs an update to the CHANGELOG (and maybe another test? I'm not sure if we've a good way to test this).

Also note the test that triggers #2540 (but this - I think - fixes).

[codecov]

Codecov Report

❌ Patch coverage is 86.66667% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.82%. Comparing base (4d661d1) to head (96be30d).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2541   +/-   ##
=======================================
  Coverage   93.81%   93.82%           
=======================================
  Files          20       20           
  Lines       12216    12223    +7     
  Branches     2250     2254    +4     
=======================================
+ Hits        11461    11468    +7     
  Misses        574      574           
  Partials      181      181           

Flag	Coverage Δ
C	84.14% <86.66%> (+0.01%)	⬆️
c-python	72.74% <0.00%> (-0.07%)	⬇️
python-tests	98.58% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
Python API	98.61% <ø> (ø)
Python C interface	92.88% <ø> (ø)
C library	91.05% <86.66%> (+<0.01%)	⬆️

🚀 New features to boost your workflow:

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

[petrelharp]

Verification that (after fixing it) the new test catches the bug without this fix:

$ git checkout main lib/msprime.c
$ make
$ uv run pytest tests/test_demography.py::TestCensusEvent::test_census_at_end_time
=========================================================================================== test session starts ===========================================================================================
platform linux -- Python 3.13.12, pytest-9.1.1, pluggy-1.6.0
rootdir: /home/peter/projects/tskit-dev/msprime
configfile: pyproject.toml
plugins: xdist-3.8.0, cov-7.1.0
4 workers [1 item]      
Fatal Python error: Aborted

[petrelharp]

What does this affect? So I think it definitely is a bug, in that we're calling things different nodes that aren't really; but this only affects census nodes: it's not going to change anything about the subsequent simulation. So, it's a pretty minor bug. It'd only affect people who wanted to see how many distinct ancestors there were at a given time using a census event

I got curious: how big of an effect is this? Here's a jupyter notebook:
census_edges.ipynb
(run with uv run --with plotnine --with jupyterlab jupyter lab),
and in this simulation of 100 samples from rho=0.01 there's a lot of census nodes that have more than one segment (like, a third of them): here is all the edges below census nodes (of which there are 154), and the color is how many distinct segments a given parent census node has (55 have > 1):

[petrelharp]

(Note: one might naively hope that adding census nodes literally does not change the output of the simulation, seed-by-seed, except for the census nodes, but this turns out not to be true; I'm not actually sure why.)

[jeromekelleher]

(Note: one might naively hope that adding census nodes literally does not change the output of the simulation, seed-by-seed, except for the census nodes, but this turns out not to be true; I'm not actually sure why.)

This is almost never the case, and it's rarely worth the effort of figuring out exactly why. I wouldn't worry about it.

[jeromekelleher]

I agree, this is the correct approach. Thanks @petrelharp!

Let's drop the inaccurate comment though

[petrelharp]

This code nerd sniped me: thianks to the code pointed to by #2540 I thought I knew how to make this work with DTWF at integer times. That was wrong: the DTWF generation comes after the census event, so the problem there is elsewhere (and more complicated, looks like). But still, this last commit makes the census event a bit more general: it now won't fail if there happens to be a pre-existing node at the given time. I think this is pretty straightforward and not fraught with peril but if we are nervous I could revert to commit n-1 to just get the comment update.

[jeromekelleher]

LGTM, happy to merge once we have test coverage at a reasonable level.

[petrelharp]

Got it; we're down to expected misses:

Will merge now after a squash.