Skip to content

issue 1340: changed mrca signature to take more than 2 args. #2121

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Feb 8, 2022
Merged

issue 1340: changed mrca signature to take more than 2 args. #2121

merged 1 commit into from
Feb 8, 2022

Conversation

@savitakartik
Copy link
Contributor

@savitakartik savitakartik commented Feb 4, 2022 •
edited
Loading

Description

Thanks for contributing to tskit! ❤️
A guide to the PR process is here

changed tree.mrca signature to take more than 2 args.
added tests for the changed functionality in a new class TestMRCA.

Fixes #1340 <- Putting the issue number here will auto-close the issue when this PR is merged

PR Checklist:

  • Tests that fully cover new/changed functionality.
  • Documentation including tutorial content if appropriate.
  • Changelogs, if there are API changes. (logged to python/CHANGELOG.rst, but might have mentioned the wrong PR ID in the changelog, sorry!)

@savitakartik savitakartik changed the title issue 1340: changed mrca signature to take more than 2 args and added tests for the same. issue 1340: changed mrca signature to take more than 2 args. Feb 4, 2022
@codecov
Copy link

codecov bot commented Feb 4, 2022 •
edited
Loading

Codecov Report

Merging #2121 (a81422a) into main (6f6ee1c) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head a81422a differs from pull request most recent head 498dab3. Consider uploading reports for the commit 498dab3 to get more accurate results

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #2121   +/-   ##
=======================================
  Coverage   93.37%   93.37%           
=======================================
  Files          27       27           
  Lines       25589    25591    +2     
  Branches     1160     1161    +1     
=======================================
+ Hits        23893    23895    +2     
  Misses       1666     1666           
  Partials       30       30
Flag Coverage Δ
c-tests 92.34% <ø> (ø)
lwt-tests 89.05% <ø> (ø)
python-c-tests 72.10% <25.00%> (-0.02%) ⬇️
python-tests 98.89% <100.00%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
python/tskit/trees.py 98.05% <100.00%> (+<0.01%) ⬆️

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 6f6ee1c...498dab3. Read the comment docs.

benjeffery
benjeffery approved these changes Feb 4, 2022
View reviewed changes
Copy link
Member

@benjeffery benjeffery left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @savitakartik,not sure why the coverage is complaining. Fix the conflict and we'll see how it goes on a second run.

jeromekelleher
jeromekelleher reviewed Feb 4, 2022
View reviewed changes
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 @savitakartik! Some minor comments about the tests, but overall this is what we're looking for.

Looks like you need to resolve some conflicts and update your branch. See the this guide for help on rebasing your branch. (Don't worry if things go a bit wrong, this happens to everyone the first few times!)

python/tests/test_highlevel.py Outdated
# 0 1

def test_two_or_more_args(self):
t2 = self.t2
Copy link
Member

@jeromekelleher jeromekelleher Feb 4, 2022

Choose a reason for hiding this comment

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

There's probably not much point in assinging to a local variable here, can just do

self.tree.mrca(0, 1) == 2
# etc

@savitakartik savitakartik marked this pull request as draft February 4, 2022 18:04
@savitakartik savitakartik marked this pull request as ready for review February 6, 2022 20:17
@jeromekelleher
Copy link
Member

jeromekelleher commented Feb 7, 2022

Looks great, thanks @savitakartik. I think it's ready to go, after a squash and rebase. See the stdpopsim docs I linked to above for help on this.

@benjeffery or I would be happy to help you through the process, if you like - it is definitely confusing the first few times!

@benjeffery
Copy link
Member

benjeffery commented Feb 7, 2022

Looks great, thanks @savitakartik. I think it's ready to go, after a squash and rebase. See the stdpopsim docs I linked to above for help on this.

@benjeffery or I would be happy to help you through the process, if you like - it is definitely confusing the first few times!

Yep, happy to do it, or to talk you through it on slack/call. LMK!

@savitakartik
issue 1340: changed mrca signature to take more than 2 args and added…
498dab3 
… tests.

Closes 1340

issue 1340: incorporated PR reviewer comments.
jeromekelleher
jeromekelleher approved these changes Feb 8, 2022
View reviewed changes
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.

Good to go 🎉

@mergify mergify bot merged commit 7238acb into tskit-dev:main Feb 8, 2022
@savitakartik
Copy link
Contributor Author

savitakartik commented Feb 10, 2022

thanks for your help with my first PR, Jerome and Ben!

@savitakartik savitakartik deleted the issue_1340 branch February 10, 2022 10:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@benjeffery benjeffery benjeffery approved these changes

@jeromekelleher jeromekelleher jeromekelleher approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Change Tree.mrca signature to take > 2 args

3 participants

@savitakartik @jeromekelleher @benjeffery