Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix saving / loading of tree sequences after simplification to a subset of sampled individuals #137

Merged
merged 12 commits into from
May 11, 2023

Conversation

bodkan
Copy link
Owner

@bodkan bodkan commented May 8, 2023

An attempt at fixing #136.

@codecov
Copy link

codecov bot commented May 8, 2023

Codecov Report

Merging #137 (6c3a7e8) into main (01af510) will increase coverage by 0.30%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #137      +/-   ##
==========================================
+ Coverage   84.04%   84.35%   +0.30%     
==========================================
  Files           7        7              
  Lines        3109     3144      +35     
==========================================
+ Hits         2613     2652      +39     
+ Misses        496      492       -4     
Impacted Files Coverage Δ
R/interface.R 80.36% <100.00%> (+0.06%) ⬆️
R/tree-sequences.R 88.39% <100.00%> (+0.61%) ⬆️
R/utils.R 88.37% <100.00%> (+0.28%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@bodkan
Copy link
Owner Author

bodkan commented May 11, 2023

Merging now. From the changelog:

Fix for ts_load() failing to load slendr-produced tree sequences after they were simplified down to a smaller set of sampled individuals (reported here). The issue was caused by incompatible sizes of the sampling table (always in the same form as used during simulation) and the table of individuals stored in the tree sequence after simplification (potentially containing a smaller set of individuals than in the original sampling table). To fix this, slendr tree sequence objects now track information about which individuals are regarded as "samples" (i.e. those with symbolic names) which is maintained through simplification, serialization and loading, and used by slendr's internal machinery during join operations. (PR #137)

@bodkan
Copy link
Owner Author

bodkan commented May 11, 2023

I have a feeling there's a more elegant solution to this. It's more and more clear that a Great Internal Refactoring will have to happen, eventually. The fact that slendr started as a purely SLiM-focused thing, had tskit support bolted on after, and yet after that added support for msprime coalescent models is... starting to show.

In the meantime, there's an acute need for Real Research(tm) to be done, so an emergency fix works just fine.

@bodkan bodkan merged commit f2c61f1 into main May 11, 2023
8 checks passed
@bodkan bodkan deleted the fix-saving-simplified-ts branch June 1, 2023 08:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant