-
Notifications
You must be signed in to change notification settings - Fork 128
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
Hide BiopythonDeprecationWarnings when reading certain sequence files #1731
base: victorlin/centralize-sequence-read
Are you sure you want to change the base?
Conversation
Suggestion for this elegant solution with version switch is from @tsibley: #1727 (comment) |
augur/io/sequences.py
Outdated
@@ -19,7 +24,7 @@ def read_sequence( | |||
|
|||
def read_sequences( | |||
*paths: Iterable[str], | |||
format: str = "fasta", | |||
format: str = BIOPYTHON_FASTA_FORMAT, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't quite understand why we don't do the BIOPYTHON_FASTA_FORMAT version switch for biopython version inside this function.
Rather than requiring users (even internal ones) to pass the fasta argument, we should just rewrite fasta
-> fasta-pearson
inside the function here and note this in the docstring and our changelog.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done:
Lines 20 to 22 in 5c53256
# Allow comments in FASTA format using fasta-pearson in later biopython versions | |
if Version(version("biopython")) >= Version("1.85") and biopython_format == "fasta": | |
biopython_format = "fasta-pearson" |
augur/ancestral.py
Outdated
@@ -399,7 +399,7 @@ def run(args): | |||
aln = args.alignment | |||
ref = None | |||
if args.root_sequence: | |||
for fmt in ['fasta', 'genbank']: | |||
for fmt in [BIOPYTHON_FASTA_FORMAT, 'genbank']: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could abstract away this change here by doing the version switch inside of read_sequence(s)
helpers instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done:
Lines 20 to 22 in 5c53256
# Allow comments in FASTA format using fasta-pearson in later biopython versions | |
if Version(version("biopython")) >= Version("1.85") and biopython_format == "fasta": | |
biopython_format = "fasta-pearson" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A unit test testing that a variety of fasta's are supported would be cool maybe, we already know that our integration tests catch it, but not unit ones.
63cdf89
to
cc365cb
Compare
945011a
to
b031ce6
Compare
cc365cb
to
8fa862a
Compare
b031ce6
to
74b4f7c
Compare
8fa862a
to
cf5c8be
Compare
Instead of relying on Biopython's which are subject to change across versions.
Biopython 1.85 shows a deprecation warning when using format='fasta' with files that start with anything but '>'. The warning as-is should not be exposed to Augur users. It is not triggered when reading files with format='fasta-pearson', so this is the easiest thing to do continue accepting such files for Biopython >=1.85. This way, Augur users get consistent, backwards compatible behavior no matter the Biopython version they use.
This reverts "Temporarily disable failing test" (f5323be) which should no longer fail after previous changes.
74b4f7c
to
5c53256
Compare
cf5c8be
to
fbb56fe
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## victorlin/centralize-sequence-read #1731 +/- ##
======================================================================
+ Coverage 72.94% 73.01% +0.06%
======================================================================
Files 79 79
Lines 8316 8326 +10
Branches 1696 1698 +2
======================================================================
+ Hits 6066 6079 +13
+ Misses 1961 1959 -2
+ Partials 289 288 -1 ☔ View full report in Codecov by Sentry. |
Description of proposed changes
Biopython 1.85 shows a deprecation warning when using format='fasta' with files that start with anything but '>'.
The warning as-is should not be exposed to Augur users. It is not triggered when reading files with format='fasta-pearson', so this is the easiest thing to do continue accepting such files for Biopython >=1.85.
This way, Augur users get consistent, backwards compatible behavior no matter the Biopython version they use.
Related issue(s)
Fixes #1727
Checklist