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

Clarify default values for inference of ambiguous bases #613

Merged
merged 2 commits into from
Aug 27, 2020

Conversation

huddlej
Copy link
Contributor

@huddlej huddlej commented Aug 26, 2020

Description of proposed changes

This commit makes the following changes to clarify what the defaults are for inferring ambiguous bases both for the user and maintainers:

  • the two ambiguity flags store their values in separate variables (avoiding conflicting defaults and making the values of the variables easier to determine by reading the code)
  • the --keep-ambiguous flag has an implicit default of False because of its action type (the usual behavior for this type of opt-in flag)
  • the --infer-ambiguous flag has an explicit default of True to properly document the default behavior in the help output and communicate to us as developers that this flag is essentially redundant
  • the script determines whether to infer ambiguity based on the joint logic of the provided flags

Related issue(s)

Resolves #612

Testing

Added minimal functional tests for augur ancestral prior to making the changes in this PR, to confirm that the new code does not break the existing user interface.

Adds tests for inference of ambiguous bases for FASTA input.
This commit makes the following changes to clarify what the defaults are for inferring ambiguous bases both for the user and maintainers:

 - the two ambiguity flags store their values in separate variables (avoiding conflicting defaults and making the values of the variables easier to determine by reading the code)
 - the `--keep-ambiguous` flag has an implicit default of `False` because of its action type (the usual behavior for this type of opt-in flag)
 - the `--infer-ambiguous` flag has an explicit default of `True` to properly document the default behavior in the help output and communicate to us as developers that this flag is essentially redundant
 - the script determines whether to infer ambiguity based on the joint logic of the provided flags

Resolves #612.
@codecov
Copy link

codecov bot commented Aug 26, 2020

Codecov Report

Merging #613 into master will decrease coverage by 0.00%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #613      +/-   ##
==========================================
- Coverage   28.38%   28.37%   -0.01%     
==========================================
  Files          39       39              
  Lines        5331     5332       +1     
  Branches     1306     1306              
==========================================
  Hits         1513     1513              
- Misses       3765     3766       +1     
  Partials       53       53              
Impacted Files Coverage Δ
augur/ancestral.py 10.89% <0.00%> (-0.11%) ⬇️

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 4dd1a36...1866dc7. Read the comment docs.

@huddlej huddlej merged commit 6231998 into master Aug 27, 2020
@huddlej huddlej deleted the clarify-ancestral-inference-args branch August 27, 2020 17:05
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.

Ancestral state reconstruction infers ambiguous tip states by default, contrary to documentation
2 participants