-
Notifications
You must be signed in to change notification settings - Fork 78
Improve IBD documentation and IdentitySegments docs #3330
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3330 +/- ##
=======================================
Coverage 89.76% 89.76%
=======================================
Files 29 29
Lines 31292 31292
Branches 5738 5738
=======================================
Hits 28089 28089
Misses 1794 1794
Partials 1409 1409
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
I made the changes above and did some more here. I think it could of course be better but is good enough to go unless someone sees typos/errors. |
petrelharp
left a comment
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.
These are all changes I implemented in my commit, but some of these comments have explanation so I'm putting them in still.
docs/ibd.md
Outdated
| segments returned are the longest possible ones: for a fixed pair | ||
| ``(u, v)`` we follow the ancestral paths from ``u`` and ``v`` up the | ||
| trees and merge together adjacent genomic intervals whenever both the | ||
| MRCA ``a`` and the full ancestral paths from ``u`` and ``v`` to ``a`` | ||
| are identical. |
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.
TODO: double-check this is true. I suspect this might need some nuance at least.
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.
None of the things I checked that might be "merged" are merged - even if I split an edge, that also splits the IBD segment.
docs/ibd.md
Outdated
| by genealogical path) the IBD segments are obtained by merging together | ||
| adjacent MRCA intervals that share the same ancestor and paths to that |
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.
| by genealogical path) the IBD segments are obtained by merging together | |
| adjacent MRCA intervals that share the same ancestor and paths to that | |
| by genealogical path) the IBD segments are those | |
| that share the same ancestor and paths to that |
docs/ibd.md
Outdated
| - Passing an empty list, e.g. ``within=[]``, is allowed and simply | ||
| yields a result with zero pairs and zero segments. |
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.
Unnecessary detail.
| - Passing an empty list, e.g. ``within=[]``, is allowed and simply | |
| yields a result with zero pairs and zero segments. |
docs/ibd.md
Outdated
| no greater than ``max_time`` are returned. The time is measured in | ||
| the same units as the node times in the tree sequence (e.g., generations). |
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.
What else would it be measured in?
| no greater than ``max_time`` are returned. The time is measured in | |
| the same units as the node times in the tree sequence (e.g., generations). | |
| no greater than ``max_time`` are returned. |
Extracted from #3329