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.

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

Replace low-level tree positioning code with tsk_tree_position_t calls #2794

Closed
jeromekelleher opened this issue Jul 13, 2023 · 2 comments · Fixed by #2874
Closed

Replace low-level tree positioning code with tsk_tree_position_t calls #2794

jeromekelleher opened this issue Jul 13, 2023 · 2 comments · Fixed by #2874
Assignees
Milestone

Comments

@jeromekelleher
Copy link
Member

Moving to the next and previous tree in a sequence is currently implemented in tsk_tree_advance and seeking to a position along the sequence (from the initial state) is done in tsk_tree_seek_from_null .

We can replace this logic with calls to tsk_tree_position_t, following the patterns used in #2786.

We should do some basic performance testing to check that we haven't introduced any regressions by doing this, say by checking on

  • A large msprime simulated tree sequence
  • A large chromosome from the unified genealogy trees
  • A SARS-Cov-2 tree sequence

Note that we didn't implement seek_backwards in #2786, as I thought this would be a good exercise for someone learning how all this stuff works. The first part of closing this issue would be a commit that implements seek_backwards in Python, and adds some more testing to ensure it's correct.

Note that this doesn't address the issues in #2792, but that can be done as a follow-up, since we're not going to make performance any worse by doing it this way.

We would continue to implement seek by iteration for the non-null case, because we're less sure about the performance implications of this, and we'd need some additional testing to verify that it's correct.

@duncanMR - are you up for this?

@duncanMR
Copy link
Contributor

@jeromekelleher Yes, I'd be happy to implement seek_backwards and do some performance tests. Just to confirm: are the unit tests for seek_forward mostly complete in #2786, such that I just need to translate them to the seek_backward case?

@jeromekelleher
Copy link
Member Author

Great!

Testing is almost complete I think, but we probably need a few more tests for seeking randomly along the sequence, and changing directions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants