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

Prevent the single-node tree check from yielding "false positives" on linear trees; also fix the build #48

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

fedarko
Copy link

@fedarko fedarko commented Apr 20, 2022

Background

The check for single-node trees was previously structured as follows (where data is a Newick-format string representing a tree):

    if data.count(',') == 0:
        raise ValueError("Only trees with more than 1 node supported")

In most cases, this check works. However, a Newick file containing no commas does not indicate that this tree contains only one node -- it just indicates that this tree does not contain any branches. In the weird case where our "tree" is a linear chain of nodes without any branches, e.g. ((b:3)a:2)root:1;, this check will falsely raise an error. (I don't expect that trees like this will come up in practice often, but some of the trees in Empress' tests are structured like this -- so the current check is breaking Empress' build.)

Changes in this PR

This PR fixes this check: now it is based on the topology of the parsed tree, rather than the input Newick string's number of commas. This means that the check will be performed "later on", from the user's perspective, but single-node trees should by definition be parsed pretty quickly anyway -- so I think postponing the check is reasonable.

This PR also adds a test, test_parse_newick_linear_tree, which ensures that "linear trees" like the ones discussed here can be parsed ok.

Also, the Travis build failed for this test (see log here, which doesn't seem to show up on this PR but I did get an email about?) -- it seems like this failure is due to 1) BitArray being moved to within the bp folder, which broke make test, and 2) flake8 issues. This PR additionally resolves both of these concerns, so the Travis build now works again.

Thanks!

fedarko added 2 commits April 19, 2022 19:52
Reliant now on the topology rather than on the string, since relying
on the presence of "," yields false positives in the case of a tree
with multiple nodes in a single linear chain.
@fedarko fedarko changed the title Prevent the single-node tree check from yielding "false positives" Prevent the single-node tree check from yielding "false positives"; also fix make test Apr 20, 2022
@fedarko fedarko changed the title Prevent the single-node tree check from yielding "false positives"; also fix make test Prevent the single-node tree check from yielding "false positives" on linear trees; also fix the build Apr 20, 2022
Just a small check that linear trees somehow aren't parsed as having
*more* nodes than they should. IDK how that would even happen, but
this ensures it shouldn't at least...
@wasade
Copy link
Member

wasade commented Apr 20, 2022

Looks good, thanks! I'm surprised actions isn't running -- I'll need to look into that before merge

@wasade
Copy link
Member

wasade commented Apr 20, 2022

I'm confused about travis as the project was using gh actions as of a few weeks ago, and I haven't made any changes to its configuration since

@wasade
Copy link
Member

wasade commented Apr 20, 2022

@fedarko could you merge master? That should allow for actions to run -- I think you're the first person besides me to issue a commit since the original actions configuration, and it may have needed a little tweaking

fedarko added 3 commits April 20, 2022 16:06
To avoid confusion; the CI for iow is now on github actions
@fedarko
Copy link
Author

fedarko commented Apr 20, 2022

@wasade got it! I've merged master in, and now it looks like github actions is able to run. Looks like all of the builds are successful.

There was a .travis.yml file remaining in the repository, so -- at least for my fork of iow -- Travis was still trying to run builds, which is weird (yesterday I got emails from Travis about the builds of my fork of iow failing, and I assumed incorrectly that iow was just using Travis instead of github actions?). Anyway, I added another commit that removes .travis.yml to avoid confusion (and relieve the stress on Travis' servers, I guess :) I also added one of those fancy badges to the README that reflects the github actions CI status, to make things clearer for other devs.

Copy link
Member

@ElDeveloper ElDeveloper left a comment

Choose a reason for hiding this comment

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

Looks good to me, just one comment.

Makefile Show resolved Hide resolved
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