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

use new Isovar API but keep old logic for determining variant effects #186

Merged
merged 18 commits into from Feb 22, 2020

Conversation

iskandr
Copy link
Contributor

@iskandr iskandr commented Feb 18, 2020

This PR is an attempt to make the smallest possible change while still bringing Vaxrank over to the new Isovar API. To do this I'm leaving some redundant code in Vaxrank for e.g. determine variant effects, which can be removed in a later PR.

Also:

  • cleaning up CLI code by pulling out little helper functions
  • created run_vaxrank_from_parsed_args, which simplifies testing
  • moved logic for --version into ArgumentParser instance

Importantly this PR allows use of newer versions of pysam (>0.9), as requested in #179 and by several other people offline

After the next PR I'll start actually re-running PGV patients and comparing reports.

@coveralls
Copy link

coveralls commented Feb 19, 2020

Coverage Status

Coverage increased (+0.2%) to 88.2% when pulling 3b0be11 on use-new-isovar-api-but-old-effects-logic into d0e74d9 on master.

@iskandr iskandr changed the title WIP: use new Isovar API but keep old logic for determining variant effects use new Isovar API but keep old logic for determining variant effects Feb 19, 2020
@iskandr iskandr mentioned this pull request Feb 20, 2020
Copy link
Contributor

@julia326 julia326 left a comment

Choose a reason for hiding this comment

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

generally LGTM but a couple follow-up questions

@@ -30,7 +30,8 @@
"--mhc-predictor", "random",
"--mhc-alleles", "H2-Kb,H2-Db",
"--padding-around-mutation", "5",
"--include-mismatches-after-variant"
# TODO: figure out what happened to this argument in Isovar
Copy link
Contributor

Choose a reason for hiding this comment

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

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Found the new name, it's --count-mismatches-after-variant

"falls below this threshold. (default: %(default)s)"))

vaccine_peptide_group.add_argument(
"--num-epitopes-per-vaccine-peptide",
Copy link
Contributor

Choose a reason for hiding this comment

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

why rename this arg?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think "per peptide" is too vague given that each epitope is itself a peptide and the longer sequence from which the vaccine peptides are drawn are also peptides.

@@ -211,26 +160,29 @@ def vaccine_peptides_for_variant(
At this point, we know the variant has RNA support, as per the
Copy link
Contributor

Choose a reason for hiding this comment

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

this comment is no longer true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

return []

variant = isovar_result.variant
long_protein_fragment = MutantProteinFragment.from_isovar_result(isovar_result)
Copy link
Contributor

Choose a reason for hiding this comment

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

why rename this to "long"? is the meaning different from before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to distinguish the 35mer source sequence from the 25mer vaccine peptides below

@iskandr iskandr merged commit 653febf into master Feb 22, 2020
@iskandr iskandr deleted the use-new-isovar-api-but-old-effects-logic branch February 22, 2020 02:58
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.

None yet

3 participants