Open duncanMR opened 7 months ago
codecov[bot]
commented
7 months ago Attention: Patch coverage is 9.21053% with 69 lines in your changes are missing coverage. Please review.
Project coverage is 89.59%. Comparing base (
8b1be4f) to head (35dcda4).
jeromekelleher
commented
7 months ago That is interesting. I can see the LD calculator causing problems as that does a lot of seeking backward and forward (was the original motivation for bidirectional seeking). I don't see anything obvious wrong, but it must be something to do with sample counts.
One thing that we can at least make progress on is that I think we should keep the option of seeking linearly. So, we make a new option to seek, TSK_SEEK_LINEAR which always uses the linear seek algorithm. We'll want to expose this to Python also, so there'll be a bit of plumbing involved.
We should specify this option in the ld_calculator, as that is definitely somewhere that linear seeking makes sense.
duncanMR
commented
7 months ago That is interesting. I can see the LD calculator causing problems as that does a lot of seeking backward and forward (was the original motivation for bidirectional seeking). I don't see anything obvious wrong, but it must be something to do with sample counts.
One thing that we can at least make progress on is that I think we should keep the option of seeking linearly. So, we make a new option to
seek,TSK_SEEK_LINEARwhich always uses the linear seek algorithm. We'll want to expose this to Python also, so there'll be a bit of plumbing involved.We should specify this option in the ld_calculator, as that is definitely somewhere that linear seeking makes sense.
Okay; that makes sense to me. I'll revert seek_linear to its previous form and move the seek algorithm that uses tree_pos to a new function. I'll make the tree_pos method the default for seek, then see exactly which methods need to be changed back to linear in order for the tests to pass. Shall we call the new tree_pos method tsk_tree_seek_nonlinear?
jeromekelleher
commented
7 months ago seek_skip might be a bit more descriptive?
Description
Continuing from #2874, we want to finish moving over the tree-positioning code to use
tree_posefficiently. At the moment,tsk.tree_seekwill either calltsk_tree_seek_from_nullortsk_tree_seek_lineardepending on whether we are starting from the null tree or not.seek_linearrepeatedly callsnextorprevuntil it reaches the given position, with the direction being determined by which would cover the shortest distance.As a first pass, I've implemented
tsk_tree_seek_forwardandtsk_tree_seek_backwardand I've incorporated them intotsk_tree_seek_linear. We will need to revise some of thetest_highlevel.pyseek tests, because the direction we choose to seek along is different to the old approach in some cases. For example, we now seek forward to go from the first to the last tree in a sequence.Curiously, my implementation passes all the C tests with no memory issues detected by Valgrind, and it also passes all the
test_highlevel.pyandtest_tree_positioningtests except for the ones dependent on seeking direction. However, it has caused chaos with other Python tests, causing failures and segfaults intest_stats.pyandtest_divmat.pyamong others. The failing/crashing tests seem to be primarily be associated with LD calculations and divergence.I'm currently trying to determine whether the problems are due to an error in my implemention (most likely) or the subtle problems with the time ordering of inserted edges, discussed in #2792.
PR Checklist: