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

Replace "recombinant" clade with label "other" #45

Merged
merged 2 commits into from
Oct 31, 2023

Conversation

trvrb
Copy link
Member

@trvrb trvrb commented May 17, 2023

Description of proposed changes

With the rise of XBB viruses there are a large number of "recombinant" clades that no longer mean the same thing they used to. I think clearer and less noisy to just always collapse "recombinant" into the "other" category.

@joverlee521: I wasn't sure if you thought this needed more of a warning to users. I was basically trying to mirror how NA is replaced with other.

Testing

Testing locally gives expected results:

dropping-recombinant

Though looks like I need to fix tests. I'll look into this.

@trvrb trvrb requested a review from joverlee521 May 17, 2023 21:11
@joverlee521
Copy link
Contributor

With the rise of XBB viruses there are a large number of "recombinant" clades that no longer mean the same thing they used to. I think clearer and less noisy to just always collapse "recombinant" into the "other" category.

Is there any reason we would want to support analysis of old dates with "recombinant" as an output variant? If so, we could use --force-include-clades recombinant=other to force the collapse without hard-coding it into the script.

@joverlee521 joverlee521 mentioned this pull request Oct 31, 2023
1 task
trvrb and others added 2 commits October 31, 2023 14:00
With the rise of XBB viruses there are a large number of "recombinant" clades that no longer mean the same thing they used to. I think clearer and less noisy to just always collapse "recombinant" into the "other" category.
@joverlee521 joverlee521 force-pushed the collapse-recombinants branch from 3e27ece to 2a0546f Compare October 31, 2023 21:02
@joverlee521 joverlee521 marked this pull request as ready for review October 31, 2023 21:08
@joverlee521
Copy link
Contributor

Ran model with clade counts produced by the test run and uploaded to s3://nextstrain-data/files/workflows/forecasts-ncov/trial/collapse-recombinants/gisaid/nextstrain_clades/global/mlr/2023-10-31_results.json

Comparing current results from 2023-10-30 with the trial results :

Screenshot 2023-10-31 at 2 56 40 PM
Screenshot 2023-10-31 at 2 56 58 PM

Screenshot 2023-10-31 at 2 57 10 PM
Screenshot 2023-10-31 at 2 57 22 PM

Copy link
Member

@jameshadfield jameshadfield left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This direction (and screenshots) looks good to me, and I think this is a better approach than #69. I'm not familiar with the code here, however I've read through the changes and they look good.

@joverlee521 joverlee521 merged commit eea90ba into main Oct 31, 2023
5 checks passed
@joverlee521 joverlee521 deleted the collapse-recombinants branch October 31, 2023 22:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

3 participants