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.

Sign up for GitHub

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

Jump to bottom

[query] In Query-on-Batch, the calculation for IBD is incorrect #14052

Closed
jigold opened this issue Nov 29, 2023 · 1 comment · Fixed by #14062
Closed

[query] In Query-on-Batch, the calculation for IBD is incorrect #14052

jigold opened this issue Nov 29, 2023 · 1 comment · Fixed by #14062
Assignees
@jigold
Labels
bug query

Comments

@jigold
Copy link
Contributor

jigold commented Nov 29, 2023

What happened?

We're dividing by X in two places when it should be Y for computing E10.

https://hail.zulipchat.com/#narrow/stream/123010-Hail-Query-0.2E2-support/topic/P.28I.3D1.7CZ.3D0.29.20computation.20in.20IBD/near/403886796

This is a trivial fix, but I want to make sure the value for E11 is also correct. We're using T/2 where as in the paper it's 1.

Regardless, we need better tests for IBD that would have caught this error.

Version

0.2.126

Relevant log output

No response
@jigold jigold added the needs-triage A brand new issue that needs triaging. label Nov 29, 2023
@danking danking changed the title Bad IBD implementation in QoB [query] In Query-on-Batch, the calculation for IBD is incorrect Nov 30, 2023
@danking danking added bug query and removed needs-triage A brand new issue that needs triaging. labels Nov 30, 2023
@jigold
Copy link
Contributor Author

jigold commented Dec 4, 2023

We weren't actually running the tests in QoB previously. I enabled the tests in #14062, but they all still passed even with the error.

PASSED test/hail/methods/relatedness/test_identity_by_descent.py::test_ibd_default_arguments
PASSED test/hail/methods/relatedness/test_identity_by_descent.py::test_ibd_does_not_error_with_dummy_maf_float64
PASSED test/hail/methods/relatedness/test_identity_by_descent.py::test_ibd_0_and_1
PASSED test/hail/methods/relatedness/test_identity_by_descent.py::test_ibd_does_not_error_with_dummy_maf_float32

My guess is our test suite isn't robust enough. I don't think we test with any family relationships -- all unrelated samples.

@jigold jigold self-assigned this Dec 4, 2023
@jigold jigold mentioned this issue Jan 3, 2024
Merged
danking pushed a commit that referenced this issue Jan 9, 2024
@jigold
[qob] Fix IBD and enable tests (#14062)
965f102 
CHANGELOG: Fixed bugs in the identity by descent implementation for
Query on Batch

This PR fixes #14052. There were two bugs in how we compute IBD. In
addition, the tests weren't running in QoB and the test dataset we were
using doesn't have enough variability to catch errors. I used Balding
Nichols generated data instead. Do we need to set the seed in the tests
here?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jigold jigold

Labels
bug query
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[qob] Fix IBD and enable tests jigold/hail
2 participants
@danking @jigold