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

Add mrca, branch_length and kc_distance functions with tests #8

Closed
wants to merge 2 commits into from

Conversation

Billyzhang1229
Copy link
Contributor

✅ mrca
✅ branch_length
✅ kc_distance

Copy link
Owner

@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.

LGTM, a few minor comments.

We don't need to get too rigid about keeping the tskit dependencies minimised, but things like num_nodes are well defined in terms of the parent_array.

phylokit/distance.py Show resolved Hide resolved
phylokit/distance.py Outdated Show resolved Hide resolved
tests/test_distance.py Outdated Show resolved Hide resolved
@jeromekelleher
Copy link
Owner

Looks like you need to rebase - as a rule of thumb, you never merge especially when git suggests you do!

@jeromekelleher
Copy link
Owner

Looks like you're not having much fun with the rebasing @Billyzhang1229 - how about Ben or I take you through it?

@Billyzhang1229
Copy link
Contributor Author

Looks like you're not having much fun with the rebasing @Billyzhang1229 - how about Ben or I take you through it?

True, I'll probably try a few more times today, I need more practice on it and reach out to you or Ben if I can't solve it by tomorrow.🙃

Add check_node_bounds() functions

Remove duplicate test in test_distance.py
@benjeffery
Copy link
Collaborator

As another git tip, it is best to work on a topic branch rather than your main as you'll often have more than one branch in progress, it also prevents you updating your main from the upstream repo.

@Billyzhang1229
Copy link
Contributor Author

As another git tip, it is best to work on a topic branch rather than your main as you'll often have more than one branch in progress, it also prevents you updating your main from the upstream repo.

I see, it is my first time to fork an upstream repository and work on it, normally I would just branch in my repository and merge it to main after finishing up.

@benjeffery
Copy link
Collaborator

As another git tip, it is best to work on a topic branch rather than your main as you'll often have more than one branch in progress, it also prevents you updating your main from the upstream repo.

I see, it is my first time to fork an upstream repository and work on it, normally I would just branch in my repository and merge it to main after finishing up.

There's loads more tips at https://tskit.dev/tskit/docs/stable/development.html#git-workflow if you haven't already seen it.

@jeromekelleher
Copy link
Owner

This has conflicts now @Billyzhang1229 - we can go through how to do this tomorrow.

@benjeffery
Copy link
Collaborator

Closed in favour of #15 which has a named branch.

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.

3 participants