Use augur subsample - #103
Conversation
e869e31 to
18232af
Compare
18232af to
b0d6728
Compare
|
I'll wait for a decision in nextstrain/public#27 before continuing here. |
b0d6728 to
5ee2efa
Compare
5ee2efa to
5490644
Compare
5490644 to
42aa5f6
Compare
42aa5f6 to
b437074
Compare
a727f0c to
9b071fb
Compare
e48b466 to
5741f0f
Compare
a28275d to
3972508
Compare
3ca4831 to
5093083
Compare
joverlee521
left a comment
joverlee521
left a comment
There was a problem hiding this comment.
Thank you for continuing to push through this! I've only added small comments for docs/changelog edits.
| ## 2026 | ||
|
|
||
| * TBD: Phylogenetic workflow configuration for initial quality filtering has moved to its own section, `filter_for_pre_subsample_alignment`. | ||
| * TBD: The `filter` section in phylogenetic workflow configuration has been replaced by `subsample`/`custom_subsample` for subsampling, and `filter_for_pre_subsample_alignment` for initial quality filtering. **This is a breaking change**. |
There was a problem hiding this comment.
This is another instance where proximity subsampling is not supported so we should note it in the config/changelog (similar to rubella).
We're going to continuously run into this issue if we don't reorder workflows to align before subsample...I wonder if we should build a way to flag this in the subsample schema validation?
There was a problem hiding this comment.
Noted in changelog:
Line 8 in f9c14c2
I wonder if we should build a way to flag this in the subsample schema validation?
How would this work? Aligned and unaligned are both FASTA files. We'd have to inspect the actual sequences, and even then it's still a guess. The context of whether a FASTA is aligned or not lives at the workflow level.
There was a problem hiding this comment.
Ah yeah, I'm thinking in the schema validation side only.
I think this would require making two schemas for augur subsample, one that includes proximalSampleProperties and the other without proximalSampleProperties. Then in the RSV workflow config schema, we'd reference the subsample schema that does not include proximalSampleProperties.
There was a problem hiding this comment.
Done using a def inside the existing subsample config schema:
Lines 89 to 96 in 54f6fd9
This needed two additional PRs:
f9c14c2 to
12efdc8
Compare
12efdc8 to
35d0ca8
Compare
35d0ca8 to
54f6fd9
Compare
There was a problem hiding this comment.
c33cf92 is a significant change, basically option (1) from nextstrain/public#23.
Similar to "Add separate frequencies config" (0b22185), the filter_for_pre_subsample_alignment rule shouldn't rely on config from another rule.
The previous subsampling implementation was fixed to a two-sample recent+background split with some hardcoded parameters. Replacing it with augur subsample allows for more flexible configuration. In Snakemake, implementation is mostly copied from pathogen repos that have switched over to augur subsample. One notable difference is that the combine_samples rule must stay to handle output from the enrich_antibody_escape rule. In the config YAML, the subsampling configuration is much more verbose as a byproduct of increased flexibility. It was generated using a script, which I'll add in another commit since it makes additional changes.
The script restores some of the logic that was originally in the hardcoded filter implementation, allowing easier bulk edits to the subsample config. One downside is that the generated file is less readable with a strict YAML style and no comments. Comments have been moved to the script, but ideally they'd live in a schema which is used to generate user-facing docs.
The new name makes it more obvious that this rule is only used when build_name=F-antibody-escape.
54f6fd9 to
d96e18a
Compare
4 participants
Note
Needs changes from nextstrain/augur#2038
Description of proposed changes
The previous subsampling implementation was fixed to a two-sample recent+background split with some hardcoded parameters. Replacing it with augur subsample allows for more flexible configuration.
In Snakemake, implementation is mostly copied from pathogen repos that have switched over to augur subsample. One notable difference is that the combine_samples rule must stay to handle output from the enrich_antibody_escape rule.
This is a breaking change and the old configuration will no longer work.
Related issue(s)
Closes #101
Checklist
old implementations