tskit-dev / tskit

Population-scale genomics
MIT License
147 stars 70 forks source link

Add tree_pos to tsk_tree_t struct #2867

Closed duncanMR closed 7 months ago

duncanMR commented 8 months ago

Description

This PR aims to address the first part of #2795 by adding the tree_position class to the tsk_tree_t struct. I've done a first draft of this and it passes all the unit tests without any memory issues detected by Valgrind. I'm not confident that the memory management is correct yet, though!

PR Checklist:

duncanMR commented 8 months ago

@jeromekelleher I see why you made the comment about doxygen in the trees header file: moving tsk_position_t above tsk_tree_t broke the docs!

codecov[bot] commented 8 months ago

Codecov Report

Merging #2867 (b72b3c2) into main (813f4ed) will decrease coverage by 12.68%. The diff coverage is 50.00%.

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/tskit-dev/tskit/pull/2867/graphs/tree.svg?width=650&height=150&src=pr&token=7RoAMA56V2&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=tskit-dev)](https://app.codecov.io/gh/tskit-dev/tskit/pull/2867?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=tskit-dev) ```diff @@ Coverage Diff @@ ## main #2867 +/- ## =========================================== - Coverage 89.69% 77.02% -12.68% =========================================== Files 30 30 Lines 30159 30146 -13 Branches 5860 5596 -264 =========================================== - Hits 27052 23220 -3832 - Misses 1778 5538 +3760 - Partials 1329 1388 +59 ``` | [Flag](https://app.codecov.io/gh/tskit-dev/tskit/pull/2867/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=tskit-dev) | Coverage Δ | | |---|---|---| | [c-tests](https://app.codecov.io/gh/tskit-dev/tskit/pull/2867/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=tskit-dev) | `86.07% <50.00%> (-0.03%)` | :arrow_down: | | [lwt-tests](https://app.codecov.io/gh/tskit-dev/tskit/pull/2867/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=tskit-dev) | `80.78% <ø> (ø)` | | | [python-c-tests](https://app.codecov.io/gh/tskit-dev/tskit/pull/2867/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=tskit-dev) | `67.89% <ø> (ø)` | | | [python-tests](https://app.codecov.io/gh/tskit-dev/tskit/pull/2867/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=tskit-dev) | `?` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=tskit-dev#carryforward-flags-in-the-pull-request-comment) to find out more. | [Files](https://app.codecov.io/gh/tskit-dev/tskit/pull/2867?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=tskit-dev) | Coverage Δ | | |---|---|---| | [c/tskit/trees.c](https://app.codecov.io/gh/tskit-dev/tskit/pull/2867?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=tskit-dev#diff-Yy90c2tpdC90cmVlcy5j) | `90.13% <50.00%> (-0.04%)` | :arrow_down: | ... and [16 files with indirect coverage changes](https://app.codecov.io/gh/tskit-dev/tskit/pull/2867/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=tskit-dev) ------ [Continue to review full report in Codecov by Sentry](https://app.codecov.io/gh/tskit-dev/tskit/pull/2867?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=tskit-dev). > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=tskit-dev) > `Δ = absolute (impact)`, `ø = not affected`, `? = missing data` > Powered by [Codecov](https://app.codecov.io/gh/tskit-dev/tskit/pull/2867?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=tskit-dev). Last update [813f4ed...b72b3c2](https://app.codecov.io/gh/tskit-dev/tskit/pull/2867?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=tskit-dev). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=tskit-dev).
duncanMR commented 8 months ago

@jeromekelleher, I've added a commit with all your suggested changes. The docs are still failing after moving all the methods to the bottom of trees.h. Doxygen runs successfully, and it seems like the xml file for the tsk_tree_t class it generates is correct. Also, the tree section of docs/_build/html/c-api.html looks correct to me; I'm confused as to what is causing the warning.

jeromekelleher commented 7 months ago

I think the docs build is working @duncanMR - I just checked it out and the build is fine for me. Also the CI docs build is working.

I think the CI failures are due to a stale package cache. Probably simplest to just start a new PR from a fresh branch.

duncanMR commented 7 months ago

I think the docs build is working @duncanMR - I just checked it out and the build is fine for me. Also the CI docs build is working.

I think the CI failures are due to a stale package cache. Probably simplest to just start a new PR from a fresh branch.

Yes, I figured out the docs problem: there is a hardcoded exception in the Sphinx config specifically for the tree class that needed an update. I'll make a new PR now

jeromekelleher commented 7 months ago

I just saw a "merge" here @duncanMR - merge is not your friend! Rebase rebase rebase

duncanMR commented 7 months ago

@jeromekelleher Sorry about that! I accidentally pulled changes instead of just fetching. Will be more careful in future

jeromekelleher commented 7 months ago

@jeromekelleher Sorry about that! I accidentally pulled changes instead of just fetching. Will be more careful in future

No need to apologise, just trying to help