-
Notifications
You must be signed in to change notification settings - Fork 130
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
[refine] prune outgroup #1751
[refine] prune outgroup #1751
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1751 +/- ##
==========================================
+ Coverage 73.11% 73.25% +0.14%
==========================================
Files 79 79
Lines 8350 8376 +26
Branches 1704 1706 +2
==========================================
+ Hits 6105 6136 +31
+ Misses 1958 1954 -4
+ Partials 287 286 -1 ☔ View full report in Codecov by Sentry. |
701d6ee
to
a914d3f
Compare
Thank you for making the ideas in #340 a reality here, @jameshadfield! I see what you mean about the complexity in seasonal-flu. You're right that the reference ids differ between segments right now, because we use sequence accession as the id. Even if we renamed the reference ids to the strain name (which will be the same for all segments within a lineage), a better strategy might be to force The other potential issue is that the |
I've shifted the seasonal-flu specific conversation to nextstrain/seasonal-flu#212 |
Testing with WNVAwesome work! From a user's perspective, I tested this PR by attempting to root the WNV/lineage-1A tree using a lineage-1B outgroup and pruning it. I included a lineage-1B outgroup strain (KX394399), which was later pruned out using the new Thank you! No blockers from me from a user perspective. |
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.
Mainly reviewed for my own learning since I've been looking at the refine code recently with @kimandrews. Only left non-blocking comments.
We now root the tree outside of treetime (if possible) before instantiating a `TreeTime` / `TreeAnc` object which reduces code duplication and simplifies the logic. There should be no behavioural changes with this commit except for improved error messages.
InvalidUseOfRemoveOutgroup error handling suggested by @joverlee521 in <#1751 (comment)>
a914d3f
to
4c12c96
Compare
AugurError doesn't result in a traceback, which is what we want here. Also took the chance to improve the text slightly
InvalidUseOfRemoveOutgroup error handling suggested by @joverlee521 in <#1751 (comment)>
4c12c96
to
826859d
Compare
826859d
to
3d2ef06
Compare
Refactors
augur refine
to root trees before TreeTime is called, where possible. This allows us to easily add a--remove-outgroup
flag which works only when the tree is rooted on a single strain, and the pruning of the root is performed before any temporal inference (if applicable). This approach is the one recommended by @rneher, @emmahodcroft and @huddlej in #340.@joverlee521 I explored patching seasonal-flu to incorporate this however the structure there is a little complicated. Specifically, if the
prune_reference
rule is dropped in lieu of--remove-outgroup
in therefine
step then the reference sequences may/will be removed via thesanitize_trees
step (which is beforerefine
) as the references (often? always?) differ across segments.Closes #340