Extract header fields from variants. #114

nmousavi · 2018-02-16T18:19:32Z

Add a PTransform to extract header fields (FORMAT, INFO) form variants.
This feature is activated by a flag, and is useful when there are header
fields in variant with no definition in headers. Without this PTransform
we currently fail the pipeline in such cases.

Related to issue #61, #101, #119

Tested:
unit test, ran pipeline for 1000genome dataset

coveralls · 2018-02-16T18:22:48Z

Pull Request Test Coverage Report for Build 332

117 of 128 (91.41%) changed or added relevant lines in 4 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage increased (+0.09%) to 90.478%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
gcp_variant_transforms/options/variant_transform_options.py	0	1	0.0%
gcp_variant_transforms/transforms/infer_undefined_headers.py	50	52	96.15%
gcp_variant_transforms/vcf_to_bq.py	2	10	20.0%

Files with Coverage Reduction	New Missed Lines	%
gcp_variant_transforms/vcf_to_bq.py	1	52.63%

Totals
Change from base Build 321:	0.09%
Covered Lines:	3031
Relevant Lines:	3350

💛 - Coveralls

arostamianfar

This is very cool! Thanks! Made some comments mainly related to naming and formatting. The design looks good overall.

arostamianfar · 2018-02-22T15:53:08Z

gcp_variant_transforms/transforms/extract_variant_header.py

+            # from variants, therefore we let the default value (True) for it
+            # be used.
+            | 'MergeHeaders' >> merge_headers.MergeHeaders(
+                split_alternate_allele_info_fields=True))


nit: since it defaults to true, you can just not specify it here. I'd even say to remove the comment.

I added the comment so we remember to change this default choice if, in future, we are able to deduce NIM=A from variants only. For the sake of clarity in the comment, I kept the arg with default value.

arostamianfar · 2018-02-22T15:53:54Z

gcp_variant_transforms/transforms/extract_variant_header.py

+        section of the VCF files. This is used to skip already defined
+        header fields.
+    """
+    self._headers = headers


Consider renaming to defined_header.

arostamianfar · 2018-02-22T16:00:47Z

gcp_variant_transforms/vcf_to_bq.py

-         | 'WriteHeaders' >> vcf_header_io.WriteVcfHeaders(
-             known_args.representative_header_file))
-
+    merged_headers = (headers


Consider renaming to merged_header since we expect one header object at this point.

arostamianfar · 2018-02-22T16:02:25Z

gcp_variant_transforms/vcf_to_bq.py

+          _read_variants(p, known_args)
+          | 'FilterVariants' >> filter_variants.FilterVariants(
+              reference_names=known_args.reference_names)
+          | 'ExtractHeaderFieldsFromVariants' >>


Consider renaming to InferUndefinedHeaderFields (I think you can drop 'FromVariants') and the return object would be inferred_headers as we should distinguish between defined and inferred.

arostamianfar · 2018-02-22T16:08:56Z

gcp_variant_transforms/vcf_to_bq.py

+                      | 'MergeHeaders' >> merge_headers.MergeHeaders(
+                          known_args.split_alternate_allele_info_fields))
+    if known_args.robust_header_extraction:
+      headers_from_variants = (


Consider extracting the body of this if as separate function that just returns a merged_header.

arostamianfar · 2018-02-22T16:22:51Z

gcp_variant_transforms/transforms/extract_variant_header.py

+        infos[info_field_key] = Info(info_field_key,
+                                     self._get_field_counts(info_field_value),
+                                     self._get_field_type(info_field_value),
+                                     _NO_DESCRIPTION,


Can we use empty strings for these?

arostamianfar · 2018-02-22T16:25:40Z

gcp_variant_transforms/transforms/extract_variant_header.py

+    for call in variant.calls:
+      for format_key, format_value in call.info.iteritems():
+        if not self._is_defined_format_field(format_key, headers):
+          assert format_key not in formats, (


Consider raising a ValueError instead of asserting (e.g. we may want to catch the error and do something with it later instead of crashing).

arostamianfar · 2018-02-22T16:25:56Z

gcp_variant_transforms/transforms/extract_variant_header.py

+      for format_key, format_value in call.info.iteritems():
+        if not self._is_defined_format_field(format_key, headers):
+          assert format_key not in formats, (
+              "Bad VCF file. Duplicate FORMAT field. %s" %format_key)


nit: s/Bad VCF file/Invalid VCF record.
And please also print the entire variant record.

arostamianfar · 2018-02-22T16:30:23Z

gcp_variant_transforms/transforms/extract_variant_header.py

+
+
+  def process(self, variant, headers):
+    return self._extract_header_fields(variant, headers)


I think the body of self._extract_header_fields is simple enough to be inlined here. That way, you can use yield instead of sometimes returning empty list as well.

arostamianfar · 2018-02-22T16:31:51Z

gcp_variant_transforms/options/variant_transform_options.py

              'even if this is provided.'))
-
+    parser.add_argument(
+        '--robust_header_extraction',


I think something like infer_undefined_headers is a more representative name for this flag.

arostamianfar

Thanks. Just a few more nits ...

arostamianfar · 2018-02-22T21:27:09Z

gcp_variant_transforms/transforms/infer_undefined_headers.py

@@ -0,0 +1,119 @@
+# Copyright 2017 Google Inc.  All Rights Reserved.


arostamianfar · 2018-02-22T21:28:50Z

gcp_variant_transforms/transforms/infer_undefined_headers.py

+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+"""A PTransform to output a PCollection of single ``HeaderFields`` extracted


s/HeaderFields/VcfHeader here and below.

arostamianfar · 2018-02-22T21:30:14Z

gcp_variant_transforms/transforms/infer_undefined_headers.py

+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+"""A PTransform to output a PCollection of single ``HeaderFields`` extracted


nit: The style guide says to have a 1-line summary for modules and then a more descriptive version below it separated by a blank line.

arostamianfar · 2018-02-22T22:24:53Z

gcp_variant_transforms/transforms/infer_undefined_headers.py

+__all__ = ['InferUndefinedHeaderFields']
+
+
+_FLAG = 'Flag'


Please create a class VcfFieldType with values like this (similar to ColumnKeyConstants in bigquery_vcf_schema).

arostamianfar · 2018-02-22T22:28:58Z

gcp_variant_transforms/transforms/infer_undefined_headers.py

+  def __init__(self, defined_headers):
+    """Initializes the transform.
+    Args:
+      defined_headers (:class:``vcf_header_io.VCFHeader``): side input


We should use the new type format, but for now, please at least add args sections for the methods so that we can change it easily later.

arostamianfar · 2018-02-22T22:29:47Z

gcp_variant_transforms/transforms/infer_undefined_headers.py

+  def __init__(self, defined_headers):
+    """Initializes the transform.
+    Args:
+      defined_headers (:class:``vcf_header_io.VCFHeader``): side input


nit: s/side/Side

arostamianfar · 2018-02-22T23:04:49Z

gcp_variant_transforms/transforms/infer_undefined_headers.py

+  def __init__(self, defined_headers):
+    """Initializes the transform.
+    Args:
+      defined_headers (:class:``vcf_header_io.VCFHeader``): side input


nit: when using :class: , you only need one backquote.

nmousavi · 2018-02-23T16:10:35Z

PTAL

arostamianfar

Just comment nits :)

arostamianfar · 2018-02-23T15:52:52Z

gcp_variant_transforms/transforms/infer_undefined_headers.py

+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+"""A PTransform to infer undefined header fields"""


nit: please add a dot at the end.

arostamianfar · 2018-02-23T15:53:19Z

gcp_variant_transforms/transforms/infer_undefined_headers.py

+    """
+    Args:
+      field_value (list, bool, integer, or string): value for the field
+        returned by pyvcf. E.g. [0.33, 0.66] is a field value for Allele freq


nit: s/pyvcf/PyVCF

arostamianfar · 2018-02-23T15:54:20Z

gcp_variant_transforms/transforms/infer_undefined_headers.py

+    """
+    Args:
+      field_value (list, bool, integer, or string): value for the field
+        returned by pyvcf. E.g. [0.33, 0.66] is a field value for Allele freq


nit: s/for Allele freq../for the allele frequency (AF) field.
(no need to imply semantics of the field here).

P.S. sorry, i went back and forth in my comment about implying the semantics lol and I just decided to reword your original comment. Please ignore my "no need to imply semantics..." comment as it doesnt make sense.

arostamianfar · 2018-02-23T15:56:28Z

gcp_variant_transforms/transforms/infer_undefined_headers.py

+      field_value (list, bool, integer, or string): value for the field
+        returned by pyvcf. E.g. [0.33, 0.66] is a field value for Allele freq
+        field (AF).
+    """


Please add Returns as well (here and below).

arostamianfar · 2018-02-23T16:33:27Z

gcp_variant_transforms/transforms/infer_undefined_headers.py

+      return VcfFieldType.STRING
+
+  def _infer_undefined_info_fields(self, variant, defined_headers):
+    """


Please add a short description as well, since it's not obvious that it only returns fields not defined in the header (for both info and format).

arostamianfar · 2018-02-23T16:34:35Z

gcp_variant_transforms/transforms/infer_undefined_headers.py

+    return infos
+
+  def _infer_undefined_format_fields(self, variant, defined_headers):
+    formats = {}


Please add args/returns here as well.

nmousavi

PTAL

Add a PTransform to extract header fields (FORMAT, INFO) form variants. This feature is activated by a flag, and is useful when there are header fields in variant with no definition in headers. Without this PTransform we currently fail the pipeline in such cases.

arostamianfar

LGTM!

nmousavi requested a review from arostamianfar February 16, 2018 18:19

nmousavi force-pushed the schema branch 4 times, most recently from 85bd40f to e4b8b4d Compare February 21, 2018 17:21

nmousavi mentioned this pull request Feb 21, 2018

Job fails on 1000Genome dataset with "No such field: call.FT." #61

Closed

arostamianfar suggested changes Feb 22, 2018

View reviewed changes

nmousavi force-pushed the schema branch 2 times, most recently from 2694de1 to 16ed0fb Compare February 22, 2018 20:33

arostamianfar suggested changes Feb 22, 2018

View reviewed changes

nmousavi force-pushed the schema branch from 16ed0fb to e6b4731 Compare February 23, 2018 15:15

arostamianfar suggested changes Feb 23, 2018

View reviewed changes

nmousavi force-pushed the schema branch from e6b4731 to 22448b2 Compare February 23, 2018 17:00

nmousavi commented Feb 23, 2018

View reviewed changes

nmousavi force-pushed the schema branch from 22448b2 to e90f143 Compare February 23, 2018 17:08

arostamianfar approved these changes Feb 23, 2018

View reviewed changes

Merge branch 'master' into schema

d389d0a

nmousavi merged commit 9de46ac into googlegenomics:master Feb 23, 2018

nmousavi deleted the schema branch April 26, 2018 18:10



		def process(self, variant, headers):
		return self._extract_header_fields(variant, headers)

		@@ -0,0 +1,119 @@
		# Copyright 2017 Google Inc. All Rights Reserved.

		__all__ = ['InferUndefinedHeaderFields']


		_FLAG = 'Flag'

Extract header fields from variants. #114

Extract header fields from variants. #114

Uh oh!

Conversation

nmousavi commented Feb 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Feb 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 332

💛 - Coveralls

Uh oh!

arostamianfar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arostamianfar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nmousavi commented Feb 16, 2018 •

edited

Loading

coveralls commented Feb 16, 2018 •

edited

Loading