-
Notifications
You must be signed in to change notification settings - Fork 13
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
Error if sgkit sample data is unphased #784
Conversation
50a744a
to
c318ca0
Compare
Codecov Report
@@ Coverage Diff @@
## main #784 +/- ##
=======================================
Coverage 93.32% 93.32%
=======================================
Files 17 17
Lines 5571 5571
Branches 990 990
=======================================
Hits 5199 5199
Misses 246 246
Partials 126 126
Flags with carried forward coverage won't be shown. Click here to find out more. 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - I guess pulling in just dask is sensible since we're already doing a bunch of behind the scenes stuff as far as sgkit is concerned.
tests/test_sgkit.py
Outdated
@@ -48,3 +50,42 @@ def test_sgkit_dataset(tmp_path): | |||
samples = tsinfer.SgkitSampleData(tmp_path / "data.zarr") | |||
inf_ts = tsinfer.infer(samples) | |||
assert np.array_equal(ts.genotype_matrix(), inf_ts.genotype_matrix()) | |||
|
|||
|
|||
def test_sgkit_unphased(tmp_path): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like four tests. Also you wouldnt' need to rmtree by doing this.
c318ca0
to
6e4bf05
Compare
@jeromekelleher Review changes made - should be good to go. |
6e4bf05
to
c1a2be0
Compare
@Mergifyio rebase |
✅ Branch has been successfully rebased |
c1a2be0
to
a7543b9
Compare
Fixes #772
This is doing unnecessary checks as not all sites may be used - but this is the simplest approach. If we think there are common use cases where used are only using a subset of sites and the rest are unphased, then could consider only checking used sites.