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

Add a --numeric-gt option to VariantsToTable #8219

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

lbergelson
Copy link
Member

@gatk-bot
Copy link

gatk-bot commented Feb 22, 2023

Github actions tests reported job failures from actions build 4246430995
Failures in the following jobs:

Test Type JDK Job ID Logs
integration 11.0.11+9 4246430995.12 logs
integration 8 4246430995.0 logs

@codecov
Copy link

codecov bot commented Feb 22, 2023

Codecov Report

Merging #8219 (c0ad2ce) into master (b4682ba) will increase coverage by 0.001%.
The diff coverage is 91.304%.

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               
Impacted Files Coverage Δ
...er/tools/walkers/variantutils/VariantsToTable.java 93.450% <84.615%> (+0.116%) ⬆️
...s/variantutils/VariantsToTableIntegrationTest.java 100.000% <100.000%> (ø)

* 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
Copy link
Collaborator

@jonn-smith jonn-smith left a 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) ) {
Copy link
Collaborator

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.

Copy link
Member Author

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...

Copy link
Collaborator

@jamesemery jamesemery left a 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";
Copy link
Collaborator

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@lbergelson
Copy link
Member Author

@jonn-smith I changed the check to be equivalent but less weird looking.
@jamesemery I verbed it. Good call.

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.

Feature request for VariantsToTable: option to preserve numeric genotypes
4 participants