Set probabilistic sampling as default subsampling behavior #659
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## master #659 +/- ##
==========================================
+ Coverage 29.31% 29.34% +0.03%
==========================================
Files 39 39
Lines 5452 5455 +3
Branches 1335 1335
==========================================
+ Hits 1598 1601 +3
Misses 3796 3796
Partials 58 58
Continue to review full report at Codecov.
|
huddlej
changed the title
huddlej
force-pushed
the
fix-probabilistic-sampling
branch
from
84fe289
to
d557157
Compare
rneher
reviewed
Jan 20, 2021
Makes the optional flag `--probabilistic-sampling` enabled by default and adds a `--no-probabilistic-sampling` flag to disable this default behavior. These changes make the `--probabilistic-sampling` flag unnecessary but maintain backwards-compatibility for workflows that reference this flag. We toggle probabilistic sampling through one variable. This simplifies the downstream logic that checks for whether to use probabilistic sampling or not. Note that with this approach we cannot define a separate `help` string for the `--no-probabilistic-sampling` flag because it will appear on the command line with a default value of its destination variable (`probabilistic_sampling=True`). This commit also disables probabilistic sampling for test builds. Test builds sample so few sequences that probabilistic subsampling can end up picking too few sequences for augur refine to work properly. This commit disables probabilistic sampling for those builds. Finally, this commit removes the redundant Zika test build implemented with Snakemake since we already have a Cram test (`zika.t`) that does the same thing.
huddlej
force-pushed
the
fix-probabilistic-sampling
branch
from
d557157
to
e1abcbb
Compare
|
Thank you, @rneher! I've updated the parser to use the |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR makes probabilistic sampling the default behavior for
augur filterwhen the user requests a maximum number of subsampling sequences.Major changes include:
--no-probabilistic-samplingflag that mirrors the existing--probabilistic-samplingflag. This flag forces augur to try to accomodate no more than the request number of sequences and throw an error if there are more subsampling groups than available sequences.--probabilistic-samplingthe default behavior when users request a maximum number of sequences to subsample. This change to the filter API allows users to define subsampling groups whose number exceeds the maximum sequences requested without throwing an error. As described in the conversation of issue Filter: Errors out when there are more groups than requested sequences #654, this new behavior is more likely the expected behavior by most users.Minor changes include:
zika.t). Since this PR revealed the annoying CI errors that can result from maintaining redundant functional tests, it is time to remove these older tests.Fixes #654