-
Notifications
You must be signed in to change notification settings - Fork 596
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
Add a --numeric-gt option to VariantsToTable #8219
base: master
Are you sure you want to change the base?
Conversation
Github actions tests reported job failures from actions build 4246430995
|
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #8219 +/- ##
===============================================
+ Coverage 86.654% 86.655% +0.001%
- Complexity 39165 39167 +2
===============================================
Files 2346 2346
Lines 183659 183673 +14
Branches 20151 20152 +1
===============================================
+ Hits 159147 159161 +14
Misses 17451 17451
Partials 7061 7061
|
* add an new option to VariantsToTable to allow output VCF style numeric GT fields previously it always output the actual bases of the Allele in the GT spot * resolves #8160 * updates htsjdk to 3.0.5
c0ad2ce
to
157d19b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One quick question about a minor refactor.
for ( final String gf : genotypeFieldsToTake ) { | ||
if ( vc.hasGenotype(sample) && vc.getGenotype(sample).hasAnyAttribute(gf) ) { | ||
if ( vc.hasGenotype(sample) && genotype.hasAnyAttribute(gf) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pulling the genotype into a variable above before doing the hasGenotype
check seems strange to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's.... a good point...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only comment is that you might consider changing the name slightly...
@@ -122,6 +123,7 @@ | |||
public final class VariantsToTable extends VariantWalker { | |||
public final static String SPLIT_MULTI_ALLELIC_LONG_NAME = "split-multi-allelic"; | |||
public final static String SPLIT_MULTI_ALLELIC_SHORT_NAME = "SMA"; | |||
public static final String NUMERIC_GT_FULLNAME = "numeric-gt"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps "use-numeric-gt" to make it verbed appropriately?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@jonn-smith I changed the check to be equivalent but less weird looking. |
previously it always output the actual bases of the Allele in the GT spot