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

Update likelihood code to handle non-uniform allelic mixtures #571

Merged
merged 10 commits into from
Sep 9, 2016

Conversation

arahuja
Copy link
Contributor

@arahuja arahuja commented Sep 8, 2016

This PR contains the following changes:

  • Misc updates from the code review on Move likelihood code to breeze #568
  • Changes Genotype from a collection of alleles to a map of Allele -> Allele Fraction
  • Updates Likelihood.scala and LikelihoodSuite.scala to handle this

This change is Reviewable

@coveralls
Copy link

Coverage Status

Coverage increased (+0.4%) to 75.589% when pulling 0dedb7e on somatic-standard-upgrades into 5924d60 on master.


lazy val uniqueAllelesCount = alleles.toSet.size
assume(alleleMixture.values.sum <= 1, "Allele fractions are larger than 1")
Copy link
Member

Choose a reason for hiding this comment

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

Will alleleMixture contain a reference allele so that we'd expect the sum to be ≈1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this test was originally = 1 but was thinking it might be more likely that we assume .5 for the reference and then something like .2 for the variant if it is a low purity sample. Could possibly come back and adjust this.

Copy link
Member

Choose a reason for hiding this comment

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

gotcha… I feel like the existence of this runtime-check raises more questions than it answers; is this a failure mode we're worried about? I'm ok leaving it in but maybe add a comment explaining the thinking, it's a peculiar and somewhat unnerving thing to feel the need to assert!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm ok, maybe to clarify - the mixture should be a true mixture where the weights sum to 1. The reason for the check is to ensure that the entered values are at least fractions ... will add some comments around this

@ryan-williams ryan-williams mentioned this pull request Sep 9, 2016
@ryan-williams
Copy link
Member

ryan-williams commented Sep 9, 2016

lgtm modulo adding a comment per above, and also some tweaks I sent in #574, which you can incorporate here or which I can pull out as a separate PR, uty!

@coveralls
Copy link

Coverage Status

Coverage increased (+0.4%) to 75.545% when pulling c79d615 on somatic-standard-upgrades into ec529d5 on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.4%) to 75.545% when pulling c79d615 on somatic-standard-upgrades into ec529d5 on master.

@arahuja arahuja merged commit b97411d into master Sep 9, 2016
@arahuja arahuja deleted the somatic-standard-upgrades branch September 9, 2016 22:24
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