Remap individuals after union #1238
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1238 +/- ##
==========================================
+ Coverage 93.74% 93.76% +0.02%
==========================================
Files 26 26
Lines 21781 21792 +11
Branches 963 963
==========================================
+ Hits 20418 20433 +15
+ Misses 1321 1318 -3
+ Partials 42 41 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
c/tskit/tables.c
Outdated
| memset(site_map, 0xff, other->sites.num_rows * sizeof(*site_map)); | ||
|
|
||
| for (k = 0; k < (tsk_id_t) other->individuals.num_rows; k++) { | ||
| individual_map[k] = TSK_NULL; |
There was a problem hiding this comment.
isn't this taken care of by the memset above?
|
This looks right to me! Thanks!! |
|
Just realised this isn't right as we should only remap the new individuals. |
|
Maybe a few test cases in Python would help clarify what is required here? |
|
Oh, right - I guess the individual parent remapping needs to happen during add_and_remap_node. Oh, wait, that won't work, because we don't know if we'll hit parent nodes before children. Gah, this stuff is terrible. Maybe the easiest thing is to separate out building the node table from building the individual table? First loop over nodes and build the individual map but don't actually add the rows to the individual table; then go through afterwards and add in the individuals? |
|
I think I have it now - remap after all the |
|
Testing this is making me think |
|
I've just pushed a commit that does this, and it required changing tests to check the metadata of individuals. Seems a good change. |
benjeffery
changed the title
AdminBot-tskit
force-pushed
the
union-remap
branch
from
19128a3 to
29eb797
Compare
|
@hyanwong getting an svg error here. Is it possible the svg isn't deterministic? |
What's the error? I can't imagine how SVG creation would fail to be deterministic, although I guess it could depend on something unexpected such as e.g. the node order. I'll double-check that the test suite uses random seeds for all the generated tree.I had some SVG changes when switching to the new msprime version, so of which seemed like differences in floating point rounding (although I have no idea why that might happen in this case) |
|
Ahhh, the nodes have individual classes! And as this PR adds individuals by default to wf sim the svg has changed. False alarm! |
|
Phew. If the sim has changed, you can overwrite the old SVG file like this: |
|
Yeah, that flag is awesome! |
4 participants
Fixes #1225.