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

added skip-scorpio parameter fixes #234 #235

Merged
merged 3 commits into from
Jun 20, 2022

Conversation

MarieLataretu
Copy link
Collaborator

  • for now the default is that scorpio is not skipped and can be skipped with pangolin_skip_scorpio

@replikation
Copy link
Owner

what is the rationale behind it?

@hoelzer
Copy link
Collaborator

hoelzer commented Jun 14, 2022

what is the rationale behind it?

This is a pangolin issue currently discussed in the community. The problem is that scorpio overwrites correct PangoLearn/Usher assignments. For example this happens for recombinants bc/ there are yet no (?) scorpio constellation files for recombinants. But this also happens for BA.4/5 which is even worse. E.g. see discussions here:

cov-lineages/scorpio#47
cov-lineages/pango-designation#713
cov-lineages/pango-designation#645
cov-lineages/scorpio#46
...

We performed some in-house tests annotation the full DESH collection w/ and w/o scorpio and the results w/o scorpio look more stable and accurate. That's why we want to add a parameter to deactivate scorpio and thus the overwrite of previous pangoLearn/User assignments.

There is also a not yet merged PR (cov-lineages/pangolin#461) about that topic. So maybe the way scorpio works will be changed anyway in the (near) future. But for now, it would be good to have an option to deactivate it.

@hoelzer
Copy link
Collaborator

hoelzer commented Jun 14, 2022

@MarieLataretu but would it be not better to have a general --additional_pango_params ? In case also other options should be given to pangolin in the future...

EDIT: although checking the way you implemented it now is also nice. It's more explicit to have a parameter for deactivation directly in the help

@MarieLataretu
Copy link
Collaborator Author

@MarieLataretu but would it be not better to have a general --additional_pango_params ? In case also other options should be given to pangolin in the future...

EDIT: although checking the way you implemented it now is also nice. It's more explicit to have a parameter for deactivation directly in the help

Yes, it's more explicit and if --additional_pango_params is per default --skip-scorpio it's kind of hard to turn it off on the command line.

Of course --additional_pango_params would be more flexible.

@replikation
Copy link
Owner

but it sounds like it would be better to deactivate it by default or did i misunderstand this?

@MarieLataretu
Copy link
Collaborator Author

I wasn't sure about changing the default behavior - I can change that to skip scorpio per default, if you agree

@hoelzer
Copy link
Collaborator

hoelzer commented Jun 14, 2022

Yeah, that's the question. We could also deactivate it by default and write that in the release notes (citing also the discussions I mentioned here)? Could be just that when the Pango devs change also the default behavior in the tool itself we also might have to change that back.

Assuming that many people just use the pipeline and report the results it might be even good we make this decision based on the current discussions and tests. So from my site also +1 to make this the current default

@replikation
Copy link
Owner

i mean most users are probably not reading this so i would just set the defaults to the best practice. :)

@MarieLataretu
Copy link
Collaborator Author

I changed the default to true.
Skipping scorpio can be deactivated with --pangolin_skip_scorpio false

@replikation
Copy link
Owner

can we changed it to just --scorpio to activate it? its more understandable for the user

@hoelzer
Copy link
Collaborator

hoelzer commented Jun 17, 2022

@replikation yeah, I like that it's not so DoppeltGemoppelt

btw the current release vs/ Maries master branch w/ scorpio deactivated:
https://twitter.com/martinhoelzer/status/1537769594546442241

@MarieLataretu
Copy link
Collaborator Author

Done.
Per default is --scorpio false, so --skip-scorpio is added to pangolin.

@replikation
Copy link
Owner

Perfect. I'll run some tests and then merge. Thank you

@replikation replikation merged commit 304938c into replikation:master Jun 20, 2022
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.

3 participants