IBD result struct #1637
IBD result struct #1637
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1637 +/- ##
==========================================
- Coverage 93.72% 93.57% -0.16%
==========================================
Files 27 27
Lines 23326 23401 +75
Branches 1084 1084
==========================================
+ Hits 21862 21897 +35
- Misses 1430 1470 +40
Partials 34 34
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
Wow. Very cool! Let me know if there's anything in particular you'd like me to look at or test out here. |
|
Yeah, this is looking promising @gtsambos, I think we'll be able to make some significant gains with this approach. I'll ping you when it's ready for a look. |
jeromekelleher
force-pushed
the
ibd_result_struct
branch
from
20a5328 to
6003920
Compare
|
This is ready for a look after #1618 goes in I think. |
jeromekelleher
force-pushed
the
ibd_result_struct
branch
from
6003920 to
abdefe5
Compare
jeromekelleher
force-pushed
the
ibd_result_struct
branch
from
abdefe5 to
c811a83
Compare
|
This is ready for review and merge now I think. There's a bunch of things not answered yet about the interface (particularly how we deal with unknown sample pairs), but I think it's best to merge this much now as a good step in the right direction. We'll need to resolve #1640 and #1639 before we can really finalise the semantics of the find_ibd operations. |
|
@Mergifyio rebase |
|
Command
|
jeromekelleher
force-pushed
the
ibd_result_struct
branch
from
c811a83 to
8da3634
Compare
|
Thanks @jeromekelleher -- today was a day off for me, I'll review this tomorrow |
There was a problem hiding this comment.
Looks good @jeromekelleher, thanks for doing this! Some of this is a bit beyond my C comprehension, but what I can understand looks good. The comments are minor, and are just about things I don't fully get.
| @@ -3404,6 +3395,12 @@ tsk_id_t tsk_table_collection_check_integrity( | |||
|
|
|||
| /* Undocumented methods */ | |||
|
|
|||
| /* TODO be systematic about where "result" should be in the params | |||
| for (j = 0; j < (int) ibd_finder.num_pairs; j++) { | ||
| ret = tsk_ibd_finder_get_ibd_segments(&ibd_finder, j, &seg); | ||
| for (j = 0; j < (int) result.num_pairs; j++) { | ||
| pair = samples + 2 * j; |
There was a problem hiding this comment.
I'm a bit confused about what this pair is supposed to be?
There was a problem hiding this comment.
It's accessing the jth pair, which is at offset 2 * j since each pair takes up two slots in the array. This is the same thing as saying pair = &samples[2 * j], as arrays are just pointer arithmetic in C.
| for (j = 0; j < (int) ibd_finder.num_pairs; j++) { | ||
| ret = tsk_ibd_finder_get_ibd_segments(&ibd_finder, j, &seg); | ||
| for (j = 0; j < (int) result.num_pairs; j++) { | ||
| pair = samples + 2 * j; |
|
Are we OK to merge this before the C release @benjeffery? I'd like to crack on and get the AVL tree code integrated ASAP so we can start finalising the semantics. |
This is a transitional implementation as we figure out the right interface for specifying how to work with large sets of IBD segments.
jeromekelleher
force-pushed
the
ibd_result_struct
branch
from
8da3634 to
ecdb685
Compare
This is stacked on #1618 so will be very noisy until that's merged
This is some initial work on refactoring the IBD code as we discussed @gtsambos. The idea is that we encapsulate all the IBD segment result information in this
tsk_ibd_result_tstruct, which we make available to the caller. All the details about how we store the segments for a pair of samples are hidden away, which gives us flexibility to do various things in the future.Still some work to be done to really make use of this, though.