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

288 Add an excess alleles param #7221

Merged
merged 8 commits into from
Apr 28, 2021
Merged

Conversation

RoriCremer
Copy link
Contributor

@RoriCremer RoriCremer commented Apr 21, 2021

Dont just use the default of 6--pass a param for the value instead (in the wdl and gatk tool)

I ran a smoke test and the filtering completed successfully, but I'm not totally sure how to make sure it used the passed in param over the default. How can I check this?

@RoriCremer RoriCremer force-pushed the rc-288-excess-alleles-param branch 3 times, most recently from 529244f to c83e99c Compare April 21, 2021 17:19
@@ -65,6 +65,12 @@
optional = true)
protected double hqGenotypeABThreshold = 0.2;

@Argument(
fullName = "excess-alleles-threshold",
doc = "excess alleles threshold", // TODO what even is this?!??!
Copy link
Member

@mmorgantaylor mmorgantaylor Apr 21, 2021

Choose a reason for hiding this comment

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

something like "number of non-reference alleles above which a site will be filtered out." ?

also @kcibul should we add an option to NOT filter these at all? maybe by setting this to +Inf or something?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say that's a nice to have... by having the threshold we are more configurable than before AND we can effectively disable it by setting this to a very large number.

@mmorgantaylor
Copy link
Member

maybe this is in progress but can you add a corresponding optional input to the GvsCreateFilterSet wdl?

@@ -65,6 +65,12 @@
optional = true)
protected double hqGenotypeABThreshold = 0.2;

@Argument(
fullName = "excess-alleles-threshold",
doc = "excess alleles threshold", // TODO what even is this?!??!
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say that's a nice to have... by having the threshold we are more configurable than before AND we can effectively disable it by setting this to a very large number.

@broadinstitute broadinstitute deleted a comment from kcibul Apr 21, 2021
@RoriCremer RoriCremer marked this pull request as ready for review April 22, 2021 17:13
@gatk-bot
Copy link

gatk-bot commented Apr 22, 2021

Travis reported job failures from build 33896
Failures in the following jobs:

Test Type JDK Job ID Logs
unit openjdk11 33896.13 logs
unit openjdk8 33896.3 logs

@gatk-bot
Copy link

gatk-bot commented Apr 23, 2021

Travis reported job failures from build 33917
Failures in the following jobs:

Test Type JDK Job ID Logs
unit openjdk11 33917.13 logs
unit openjdk8 33917.3 logs
unit openjdk11 33917.13 logs
unit openjdk8 33917.3 logs

@gatk-bot
Copy link

gatk-bot commented Apr 27, 2021

Travis reported job failures from build 33966
Failures in the following jobs:

Test Type JDK Job ID Logs
unit openjdk11 33966.13 logs
unit openjdk8 33966.3 logs
unit openjdk11 33966.13 logs
unit openjdk8 33966.3 logs
unit openjdk11 33966.13 logs
unit openjdk8 33966.3 logs
unit openjdk11 33966.13 logs
unit openjdk8 33966.3 logs
unit openjdk8 33966.3 logs
unit openjdk8 33966.3 logs
unit openjdk11 33966.13 logs

@gatk-bot
Copy link

gatk-bot commented Apr 28, 2021

Travis reported job failures from build 33992
Failures in the following jobs:

Test Type JDK Job ID Logs
unit openjdk11 33992.13 logs
unit openjdk8 33992.3 logs
unit openjdk8 33992.3 logs
unit openjdk8 33992.3 logs

@RoriCremer RoriCremer merged commit 66ae4b4 into ah_var_store Apr 28, 2021
@RoriCremer RoriCremer deleted the rc-288-excess-alleles-param branch April 28, 2021 19:17
This was referenced Mar 17, 2023
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.

4 participants