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

Tree path length #2259

Merged
merged 1 commit into from May 11, 2022
Merged

Tree path length #2259

merged 1 commit into from May 11, 2022

Conversation

jeremyguez
Copy link
Contributor

@jeremyguez jeremyguez commented May 10, 2022

I give here a try at #2249. I commented out the last test (copied from TestMRCA) as the trees I generated with tskit.Tree.generate_balanced did not have a virtual node (don't know why). I added some assert comparing with self.t.depth() outputs, but not sure if it's a good idea.
@jeromekelleher

@jeremyguez jeremyguez mentioned this pull request May 10, 2022
@codecov
Copy link

codecov bot commented May 10, 2022

Codecov Report

Merging #2259 (65b4e8a) into main (5bff374) will decrease coverage by 11.76%.
The diff coverage is 20.00%.

❗ Current head 65b4e8a differs from pull request most recent head 6630dc1. Consider uploading reports for the commit 6630dc1 to get more accurate results

Impacted file tree graph

@@             Coverage Diff             @@
##             main    #2259       +/-   ##
===========================================
- Coverage   93.31%   81.54%   -11.77%     
===========================================
  Files          27       27               
  Lines       26035    25894      -141     
  Branches     1170     1170               
===========================================
- Hits        24294    21116     -3178     
- Misses       1711     4711     +3000     
- Partials       30       67       +37     
Flag Coverage Δ
c-tests 92.23% <ø> (ø)
lwt-tests 89.05% <ø> (ø)
python-c-tests 71.94% <20.00%> (-0.02%) ⬇️
python-tests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
python/tskit/trees.py 47.35% <20.00%> (-50.68%) ⬇️
python/tskit/cli.py 0.00% <0.00%> (-95.94%) ⬇️
python/tskit/vcf.py 9.67% <0.00%> (-90.33%) ⬇️
python/tskit/text_formats.py 10.25% <0.00%> (-89.75%) ⬇️
python/tskit/drawing.py 10.41% <0.00%> (-89.02%) ⬇️
python/tskit/combinatorics.py 13.70% <0.00%> (-85.66%) ⬇️
python/tskit/stats.py 35.13% <0.00%> (-64.87%) ⬇️
python/tskit/util.py 56.41% <0.00%> (-43.59%) ⬇️
python/tskit/metadata.py 79.52% <0.00%> (-19.50%) ⬇️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5bff374...6630dc1. Read the comment docs.

Copy link
Member

@jeromekelleher jeromekelleher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Some minor comments, and we're good to go then.

python/tskit/trees.py Outdated Show resolved Hide resolved
python/tskit/trees.py Outdated Show resolved Hide resolved
python/tskit/trees.py Show resolved Hide resolved
python/tests/test_highlevel.py Outdated Show resolved Hide resolved
Copy link
Member

@jeromekelleher jeromekelleher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks @jeremyguez! I've one more suggestion above for testing the interface, and we're done then.

Can you squash your commits down to one then please? (See this guide if you're not familiar with squashing)

python/tests/test_highlevel.py Outdated Show resolved Hide resolved
Copy link
Member

@jeromekelleher jeromekelleher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, looking good! Just one last formatting nitpick.

Just fyi, rather than adding another commit and squashing, this is a case where "amending" a commit is handy. git commit -a --amend should update the commit, and you can then force push to update the PR.

python/tskit/trees.py Outdated Show resolved Hide resolved
python/tskit/trees.py Outdated Show resolved Hide resolved
Minor change in path_length documentation

Minor changes and added tests for path_length method

Add tests for path_length method
@jeromekelleher jeromekelleher added the AUTOMERGE-REQUESTED Ask Mergify to merge this PR label May 11, 2022
@jeromekelleher
Copy link
Member

Fantastic, thanks @jeremyguez !

@mergify mergify bot merged commit f144da6 into tskit-dev:main May 11, 2022
@mergify mergify bot removed the AUTOMERGE-REQUESTED Ask Mergify to merge this PR label May 11, 2022
@jeremyguez
Copy link
Contributor Author

Thanks for your help @jeromekelleher !

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

Successfully merging this pull request may close these issues.

None yet

2 participants