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

bugfix tree file opening in beast import #1439

Merged
merged 2 commits into from
Mar 15, 2024

Conversation

tomkinsc
Copy link
Contributor

Description of proposed changes

parse_nexus() had a block to inspect the tree argument provided and if a path string, open the file and read tree data, or if a file handle, use it directly. Comparable functionality was recently made available via the augur.io.file.open_file() context manager, which was then employed in place of open() calls across the codebase. The change from open() to open_file() broke parse_beast_tree() since augur.io.file.open_file() returns a context manager rather than the file handle expected by parse_nexus().

This commit removes the use file-or-handle logic from parse_beast_tree(), and has the function consume the file handle yielded by the augur.io.file.open_file() context manager (i.e. it adds a with open_file(...) as handle:).

bugfix beast import file opening: parse_nexus() had a block to inspect the tree argument provided and if a path string, open the file and read tree data, or if a file handle, use directly. Comparable functionality was made available via the augur.io.file.open_file() context manager, which was used in place of open() calls across the codebase. This broke parse_beast_tree() since augur.io.file.open_file() returns a context manager rather than an open file handle. This commit removes the use file-or-handle logic from parse_beast_tree(), and makes use of the file handle yielded by the context manager (i.e. it adds "with open_file(tree_path,'r') as handle:")
tomkinsc added a commit to broadinstitute/viral-pipelines that referenced this pull request Mar 15, 2024
pin nextstrain/base image to build-20240209T204939Z until a tagged release of the image includes a version of augur with the bugfix included in nextstrain/augur#1439
Copy link

codecov bot commented Mar 15, 2024

Codecov Report

Attention: Patch coverage is 0% with 26 lines in your changes are missing coverage. Please review.

Project coverage is 68.73%. Comparing base (a9572d5) to head (d8b8a3e).

❗ Current head d8b8a3e differs from pull request most recent head 1138c65. Consider uploading reports for the commit 1138c65 to get more accurate results

Files Patch % Lines
augur/import_/beast.py 0.00% 26 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1439      +/-   ##
==========================================
+ Coverage   68.67%   68.73%   +0.05%     
==========================================
  Files          69       69              
  Lines        7557     7551       -6     
  Branches     1851     1851              
==========================================
  Hits         5190     5190              
+ Misses       2089     2083       -6     
  Partials      278      278              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@joverlee521 joverlee521 left a comment

Choose a reason for hiding this comment

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

Thank you for the bug fix @tomkinsc! I added an entry in the changelog for this update. We try to avoid releases on Fridays, so I'll plan to do a release of Augur with this fix on Monday.

For our own reference, the bug was introduced in 51a77a9 and we didn't catch it because we don't have any test coverage for the augur import command.

@joverlee521 joverlee521 merged commit 4dd23c5 into nextstrain:master Mar 15, 2024
18 of 19 checks passed
@joverlee521
Copy link
Contributor

FYI @tomkinsc, this fix has been released as part of Augur 24.3.0. The latest nextstrain/base image (build-20240318T173028Z) should include the new version of Augur.

tomkinsc added a commit to broadinstitute/viral-pipelines that referenced this pull request Mar 18, 2024
update nextstrain/base image to build-20240318T173028Z, which should include a version of augur incorporating the bugfix from nextstrain/augur#1439
@victorlin
Copy link
Member

victorlin commented Mar 18, 2024

Hi @tomkinsc, thank you for handling this! I should have checked all new references to open_file() to ensure proper usage. I checked all just now. Most new references use with which is ok as a direct replacement of open(). There are changes in two that need special treatment: augur/filter/_run.py (handled properly in 51a77a9) and augur/import_/beast.py (handled in this PR).

victorlin referenced this pull request Mar 18, 2024
This allows setting defaults and supporting various compression formats
all from one central function.

Maybe a similar central function should be used for pandas.read_csv().
@tomkinsc tomkinsc deleted the ct-bugfix-beast-import branch March 22, 2024 13:48
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